| Message ID | 20181209193749.12277-8-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Plugin support | expand |
Emilio G. Cota <cota@braap.org> writes: > This is faster than removing elements one by one. > > Will gain a user soon. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > include/qemu/queue.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/qemu/queue.h b/include/qemu/queue.h > index ac418efc43..0283c2dd7d 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -419,6 +419,16 @@ struct { \ > (elm)->field.tqe_prev = NULL; \ > } while (/*CONSTCOND*/0) > > +/* 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; Although to be honest every time I read the QUEUE macros I end up with a headache... > + } while (/*CONSTCOND*/0) > + > #define QTAILQ_FOREACH(var, head, field) \ > for ((var) = ((head)->tqh_first); \ > (var); \ -- Alex Bennée
On Mon, Feb 25, 2019 at 16:22:52 +0000, Alex Bennée wrote: > Emilio G. Cota <cota@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
diff --git a/include/qemu/queue.h b/include/qemu/queue.h index ac418efc43..0283c2dd7d 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -419,6 +419,16 @@ struct { \ (elm)->field.tqe_prev = NULL; \ } while (/*CONSTCOND*/0) +/* 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; \ + } while (/*CONSTCOND*/0) + #define QTAILQ_FOREACH(var, head, field) \ for ((var) = ((head)->tqh_first); \ (var); \
This is faster than removing elements one by one. Will gain a user soon. Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/qemu/queue.h | 10 ++++++++++ 1 file changed, 10 insertions(+)