Re: [Xen-devel] [PATCH] slightly consolidate code in free_domheap_pages()

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2014-06-24 at 13:25 +0100, Jan Beulich wrote:
> >>> On 24.06.14 at 14:10, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> > On Tue, 2014-06-24 at 12:53 +0100, Jan Beulich wrote:
> >> >>> On 24.06.14 at 13:27, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> >> > On Tue, 2014-06-24 at 11:25 +0100, Jan Beulich wrote:
> >> >> >>> On 24.06.14 at 12:04, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> >> >> > If so, can d at this point ever be anything other than dom_cow or 
> >> >> > NULL?
> >> >> > I don't think so. Given that I think ASSERT(!(d == dom_cow && order !=
> >> >> > 0)) would more clearly capture the intent of the test (with the 
> >> >> > spelling
> >> >> > out of the conditions being more important than the de morganing of 
> >> >> > the
> >> >> > expression).
> >> >> 
> >> >> Indeed, d can only be NULL or dom_cow here (being in the else part
> >> >> of the if() you quoted at the top). So an alternative might indeed be
> >> >> ASSERT(d != dom_cow || !order), but that seems less desirable to
> >> >> me as it opens up ways to pass the ASSERT() with d != NULL should
> >> >> the if() condition ever get modified. I.e. I'd prefer the assertion to 
> >> >> be
> >> >> as restrictive as possible, getting relaxed only when in fact necessary.
> >> > 
> >> > Since the original if involves d == dom_cow but nothing to do with order
> >> > it seemed that the check was somehow specific to dom_cow's relationship
> >> > to higher order allocations.
> >> > 
> >> > I suppose the question is what relationship would a non-NULL d have to
> >> > the order of the allocation. i.e. if the if were changed to also
> >> > consider dom_foo why would we expect now that dom_foo had any order
> >> > requirements?
> >> 
> >> We won't know, but by having it the way it is now in the patch we're
> >> on the safe side (nothing unintended will slip through), whereas if we
> >> change to comparing against dom_cow a not sufficiently careful future
> >> change may introduce an issue.
> > 
> > That's true I . Could you add a comment though, since as it is the
> > current relationship to dom_cow and order is somewhat obscured.
> 
> That I can do,

Thanks.

>  ...
> 
> > ASSERT(d == NULL || ( d == dom_cow && !order ) ) is too much?
> 
> ... but this looks too ugly to me (namely in the else path to an if
> checking d against exactly these two values).

There's three values in the assert, the third of which is constrained by
the other two, which is rather the point. Anyway, a comment will
suffice.

Ian.


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