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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-08-13 at 03:31 -0600, Jan Beulich wrote:
> > 
> > > > 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). 

Thanks, I missed that big comment somehow.

> > 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).

The fact that act->frame/act->pin together somehow imply a reference count
(taken elsewhere?) is not all that obvious, at least to me. A comment to
that effect was exactly what I was after.

Ian.


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