On Mon, Feb 25, 2019 at 16:22:52 +0000, Alex Bennée wrote:
> Emilio G. Cota <c...@braap.org> writes:
> > +/* remove @left, @right and all elements in between from @head */
> > +#define QTAILQ_REMOVE_SEVERAL(head, left, right, field) do { \
> > + if (((right)->field.tqe_next) != NULL) \
> > + (right)->field.tqe_next->field.tqe_prev = \
> > + (left)->field.tqe_prev; \
> > + else \
> > + (head)->tqh_last = (left)->field.tqe_prev; \
> > + *(left)->field.tqe_prev = (right)->field.tqe_next; \
>
> This seems wrong, shouldn't it be:
>
> (left)->field.tqe_prev->field.tqe_next = (right)->field.tqe_next;
That would make too much sense, wouldn't it!
Looking at QTAILQ_REMOVE is the easiest way to reason about this:
#define QTAILQ_REMOVE(head, elm, field) do { \
if (((elm)->field.tqe_next) != NULL) \
(elm)->field.tqe_next->field.tqe_prev = \
(elm)->field.tqe_prev; \
else \
(head)->tqh_last = (elm)->field.tqe_prev; \
*(elm)->field.tqe_prev = (elm)->field.tqe_next; \
(elm)->field.tqe_prev = NULL; \
} while (/*CONSTCOND*/0)
Imagine we're removing the only element in the list. Thus,
*(elm)->field.tqe_prev will be setting (head)->tqh_first (to NULL).
IOW, we can't assume that the previous "element" (whether true element
or head) has a .tqe_next field (the head has .thq_first); note that
tqe_prev is a **, not a *.
> Although to be honest every time I read the QUEUE macros I end up with a
> headache...
You're not alone.
BTW, this code had to change for v3 because of 7274f01bb8 ("qemu/queue.h:
reimplement QTAILQ without pointer-to-pointers", 2019-01-11)
I think you might like that implementation better, since it is a little
closer to sanity, i.e. kernel-style lists :-)
It uses a more predictable structure (*prev,*next) that is shoehorned
with a union to avoid changing the memory layout.
Thanks,
Emilio
