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

[Stefan Hajnoczi]

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.

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)

signature.asc
Description: PGP signature