Re: [PATCH] fix page_list_splice()

[Keir Fraser <keir.xen@gmail.com>]

On 06/06/2012 10:26, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 06.06.12 at 11:02, Keir Fraser <keir@xen.org> wrote:
>> On 06/06/2012 09:23, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> Other than in __list_splice(), the first element's prev pointer doesn't
>>> need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
>>> the assignment (to formally match __list_splice()), simply assert that
>>> this assignment is really unnecessary.
>>> 
>>> Reported-by: Jisoo Yang <jisooy@gmail.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> 
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
>>>      last = list->tail;
>>>      at = head->next;
>>>  
>>> -    first->list.prev = page_to_pdx(head->next);
>>> +    /* 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?

 -- Keir

> Jan
>