Re: [Xen-devel] [PATCH v9 4/5] x86/mem_sharing: reset a fork

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

> >> In such a case, please put in a comment explaining why failure is
> >> impossible. In the general case e.g. a 2Mb page may need splitting,
> >> which may yield -ENOMEM. Such a comment will then also be useful in
> >> case a new failure mode gets added to ->set_entry(), where it then
> >> will need judging whether the assumption here still holds. (This is
> >> also why in general it'd be better to handle the error. It'll still
> >> be better to crash the guest than the host in case you can't. See
> >> the bottom of ./CODING_STYLE.)
> >
> > The mem_sharing codebase uses ASSERT(!rc) on p2m->set_entry already
> > when removing pages like we do here (see relinquish_shared_pages).
> > This only gets called on shared pages that we know for sure are
> > present. Since these are shared pages we know that their size is 4k
> > thus there is no splitting. I can certainly add a comment to this
> > effect to spell it out why the ASSERT is appropriate.
>
> Well, if this is a pre-existing pattern in the file, then - you
> being the maintainer - so be it. I consider this bad practice though,
> and I would suggest that every such site needs a comment (possibly
> all but one simply referring to the one where things get actually
> explained).
>

Noted. I think for an experimental code-base it's fine.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel