Re: [PATCH] blkdebug: ignore invalid rules in non-coroutine context

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/13/22 12:56, Markus Armbruster wrote:
>> rule_check() is called from blkdebug_co_preadv(), blkdebug_co_pwritev(),
>> blkdebug_co_pwrite_zeroes(), blkdebug_co_pdiscard(),
>> blkdebug_co_block_status() (all marked coroutine_fn), and
>> blkdebug_co_flush() (which looks like it should be marked coroutine_fn).
>
> Yes (separate patch sent, 
> 20221013123711.620631-11-pbonzini@redhat.com/T/#u">https://lore.kernel.org/qemu-devel/20221013123711.620631-11-pbonzini@redhat.com/T/#u).
>
>> Ignorant question: how could it be called outside coroutine context?
>
> You're right, only blkdebug_debug_event() can be called outside coroutine 
> context.  I confused process_rule() (called by 
> blkdebug_debug_event(), both inside and outside coroutine context) with 
> rule_check() (called in coroutine context).

Let's drop the rule_check() hunk then.

>> Also, code smell: reporting an error without taking an error path.  But
>> let's worry about that only after I understand the problem you're trying
>> to fix.
>
> Unfortunately there's no way to know in advance if an event will be called 
> inside vs. outside a coroutine.  I can keep the abort() if you 
> think it's preferrable, so what you get is still a crash but with a nicer 
> error message.  Since this is debugging code either solution has 
> pros and cons.

Let's have another look at the remaining patch hunk:

    @@ -858,7 +864,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
         }

         while (actions_count[ACTION_SUSPEND] > 0) {
    -        qemu_coroutine_yield();
    +        if (qemu_in_coroutine()) {
    +            qemu_coroutine_yield();
    +        } else {
    +            error_report("Non-coroutine event %s cannot suspend\n",
    +                         BlkdebugEvent_lookup.array[event]);
    +        }
             actions_count[ACTION_SUSPEND]--;
         }
     }

If I understand this correctly, the user asked us to suspend, but it now
turns out suspend doesn't make sense, so we ignore the request.
Correct?

warn_report()?  info_report()?