Re: [PATCH v4 1/6] blkdebug: refactor removal of a suspended request

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 07/06/2021 11:23, Paolo Bonzini wrote:
> On 04/06/21 18:16, Eric Blake wrote:
>> On Fri, Jun 04, 2021 at 12:07:36PM +0200, Emanuele Giuseppe Esposito 
>> wrote:
>>> Extract to a separate function.  Do not rely on FOREACH_SAFE, which is
>>> only "safe" if the *current* node is removed---not if another node is
>>> removed.  Instead, just walk the entire list from the beginning when
>>> asked to resume all suspended requests with a given tag.
>>> -    QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, next) {
>>> +retry:
>>> +    QLIST_FOREACH(r, &s->suspended_reqs, next) {
>>>           if (!strcmp(r->tag, tag)) {
>>> +            QLIST_REMOVE(r, next);
>>
>> Isn't the whole point of using QLIST_FOREACH_SAFE the ability to call
>> QLIST_REMOVE on an element in that list while still iterating?
>>
>>>               qemu_coroutine_enter(r->co);
>>> +            if (all) {
>>> +                goto retry;
>>> +            }
>>>               return 0;
>>
>> Oh, I see - you abandon the iteration in all control flow paths, so
>> the simpler loop is still okay.  Still, this confused me enough on
>> first read that it may be worth a comment, maybe:
>>
>> /* No need for _SAFE, because iteration stops on first hit */
> 
> This is a bit confusing too because it sounds like not using _SAFE is an 
> optimization, but it's actually wrong (see commit message).
> 

What about:

/* No need for _SAFE, since a different coroutine can remove another 
node (not the current one) in this list, and when the current one is 
removed the iteration starts back from beginning anyways. */

Alternatively, no comment at all.

Thank you,
Emanuele