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

[Philippe Mathieu-Daudé]

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, address@hidden 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)