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

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.

Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blkdebug.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

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.
> 
> Co-developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/blkdebug.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 2c0b9b0ee8..8f19d991fa 100644
> --- a/block/blkdebug.c

> -static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
> +static int resume_req_by_tag(BDRVBlkdebugState *s, const char *tag, bool all)
>  {
> -    BDRVBlkdebugState *s = bs->opaque;
> -    BlkdebugSuspendedReq *r, *next;
> +    BlkdebugSuspendedReq *r;
>  
> -    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 */

Reviewed-by: Eric Blake <eblake@redhat.com>

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).

Paolo

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

On Tue, Jun 08, 2021 at 10:00:01AM +0200, Emanuele Giuseppe Esposito wrote:
> > > 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. */

Works for me, although you'll have to reformat it to pass syntax check.