Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/25/20 10:05 AM, Stefan Hajnoczi wrote:
> On Mon, Feb 24, 2020 at 12:54:37PM +0100, Philippe Mathieu-Daudé wrote:
>> On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
>>> On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
>>>> === OUTPUT BEGIN ===
>>>> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
>>>> ERROR: do not use assignment in if condition
>>>> #65: FILE: include/qemu/queue.h:314:
>>>> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>>>>
>>>> total: 1 errors, 0 warnings, 59 lines checked
>>>
>>> The same pattern is used elsewhere in this file.  This code comes from
>>> BSD and doesn't comply with QEMU's coding style.
>>
>> Checkpatch is right, assigning out of the if statement makes the review
>> easier, and we can avoid the 'elm' null deref:
> 
> The rest of the file uses if ((a = b) == NULL), so making it
> inconsistent in this one instance isn't very satisfying.
> 
>> #define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
>> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
>> +    typeof((head)->sqh_first) elm = (head)->sqh_first; \
>> +    (head)->sqh_first = elm->field.sqe_next; \
>> +    if (elm == NULL) { \
> 
> The previous line would have segfaulted if elm was NULL so this check
> doesn't make sense.
> 
> This macro assumes there is at least one element in the list.

Ah good point, thanks.

> 
> The point of the check is to fix up the sqh_last pointer in the head
> when the final element is removed from the list.
> 
>>           (head)->sqh_last = &(head)->sqh_first; \
>> +    } else { \
>> +        elm->field.sqe_next = NULL; \
>> +    } \
>>   } while (/*CONSTCOND*/0)