Re: [Xen-devel] [PATCH] add page_get_owner_and_reference() related ASSERT()s

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.08.15 at 10:50, <ian.campbell@xxxxxxxxxx> wrote:
> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
>> The function shouldn't return NULL after having obtained a reference,
>> or else the caller won't know to drop it.
> 
>> Also its result shouldn't be ignored - if calling code is certain that
>> a page already has a non-zero refcount, it better ASSERT()s so.
> 
> How is page_get_owner_and_reference sure the reference count was not zero
> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
> itself) Or have I misunderstood the assertion being made here?

There is still a NULL return path left (when the reference couldn't
be obtained, which includes the case where the reference count
was zero). 

> Likewise where does the preexisting reference in the grant table case come
> from? Perhaps a comment alongside the ASSERT would make it clearer what the
> assert was doing to future readers ("/* Reference count must have already
> been >=1 due to XXX, so this cannot have failed */") or some such.

It is my understanding that - as seen in the patch context - the
page's MFN being read from act->frame implies that (together with
the condition of the respective if() the else it's sitting in, namely the
fact that act->pin is non-zero when we get here). It would seem
odd to state in a comment what surrounding code does (I would
agree to the need of a comment if the requirement was satisfied in
a place far away).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel