Re: [Xen-devel] [PATCH] fix page_list_splice()

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Keir Fraser <keir.xen@xxxxxxxxx> 06/06/12 1:28 PM >>>
>On 06/06/2012 10:26, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>>> On 06.06.12 at 11:02, Keir Fraser <keir@xxxxxxx> wrote:
>>> On 06/06/2012 09:23, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>> +    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
>>> 
>>> ASSERT(first->list.prev == PAGE_LIST_NULL); ?
>>> 
>>> It seems odd to have one assumption encoded as an ASSERTion, and one as a
>>> code comment... A second assertion makes the assumption explicit, and
>>> run-time checked in debug builds.
>> 
>> But the __list_splice() equivalent assignment would be
>> 
>>     first->list.prev = at->list.prev;
>> 
>> which is why I chose the assert expression that way, yet made
>> the comment clarify what the actual state is. If the comment
>> just repeated what the ASSERT() already says, I'd rather drop
>> the comment altogether.
>
>I mean to replace the comment with a second assertion, not to replace the
>assertion your patch already adds. Does that make sense?

>From the perspective of the operation here, we really just want to make
sure that the new list head's prev points to where the old one's pointed;
the fact that they're both supposed to be PAGE_LIST_NULL doesn't really
matter. Hence simply asserting that the assignment is superfluous seems
the best choice to me, with the comment explaining why. If you have
suggestions for better wording of the comment, I'll gladly take those.

The second best choice imo would be to just have the superfluous
assignment there (making it likely that later someone will come and
ask why it's there when not really needed).

Jan


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