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
+++ b/block/blkdebug.c
@@ -793,7 +793,6 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
printf("blkdebug: Resuming request '%s'\n", r.tag);
}
- QLIST_REMOVE(&r, next);
g_free(r.tag);
}
@@ -869,25 +868,35 @@ static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,
return 0;
}
-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);
qemu_coroutine_enter(r->co);
+ if (all) {
+ goto retry;
+ }
return 0;
}
}
return -ENOENT;
}
+static int blkdebug_debug_resume(BlockDriverState *bs, const char *tag)
+{
+ BDRVBlkdebugState *s = bs->opaque;
+
+ return resume_req_by_tag(s, tag, false);
+}
+
static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
const char *tag)
{
BDRVBlkdebugState *s = bs->opaque;
- BlkdebugSuspendedReq *r, *r_next;
BlkdebugRule *rule, *next;
int i, ret = -ENOENT;
@@ -900,11 +909,8 @@ static int blkdebug_debug_remove_breakpoint(BlockDriverState *bs,
}
}
}
- QLIST_FOREACH_SAFE(r, &s->suspended_reqs, next, r_next) {
- if (!strcmp(r->tag, tag)) {
- qemu_coroutine_enter(r->co);
- ret = 0;
- }
+ if (resume_req_by_tag(s, tag, true) == 0) {
+ ret = 0;
}
return ret;
}
--
2.30.2
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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.