Re: [Qemu-devel] [PATCH] rng: switch request queue to QSIMPLEQ

[Amit Shah <amit.shah@redhat.com>]

On (Fri) 04 Mar 2016 [10:27:57], Paolo Bonzini wrote:
> 
> 
> On 04/03/2016 10:19, Ladi Prosek wrote:
> > On Fri, Mar 4, 2016 at 10:12 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>
> >> On 04/03/2016 09:04, Ladi Prosek wrote:
> >>>>>>> +    QSIMPLEQ_INIT(&s->requests);
> >>>>>>>  }
> >>>>>
> >>>>> This init here isn't necessary, the accessors for the queue will take
> >>>>> care of this.
> >>> We are basically purging the queue here and we want to leave it in a
> >>> consistent state. Without the QSIMPLEQ_INIT the queue head would
> >>> become a pair of dangling pointers. Let me know if I misunderstood
> >>> your comment.
> >>
> >> It wouldn't, check out QSIMPLEQ_REMOVE_HEAD:
> >>
> >> #define QSIMPLEQ_REMOVE_HEAD(head, field) do {
> >>     if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)
> >>         (head)->sqh_last = &(head)->sqh_first;
> >> } while (/*CONSTCOND*/0)
> >>
> >> The queue would become { NULL, &s->requests.sqh_first }.  So the
> >> QSIMPLEQ_INIT is indeed redundant.
> > 
> > Right, but we're not running QSIMPLEQ_REMOVE_HEAD in this function. We
> > iterate the queue and free all elements without writing anything to
> > the head or to the next ptr. This is the only "write" we do in
> > rng_backend_free_requests.
> 
> Ah, sorry, I was convinced that rng_backend_free_request did the remove,
> but now I remember checking it yesterday (after making the same
> reasoning as Amit) and indeed it doesn't. :)
> 
> So the patch is okay.  It's just a slightly unusual use of
> QSIMPLEQ_FOREACH_SAFE.

Yeah, it's confusing when common idioms don't apply.

Nice attention to detail, Ladi :-)


		Amit