Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of BlockDriver function pointers

[Hanna Reitz <hreitz@redhat.com>]

On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block.c        | 18 ++++++++++++++++++
>   block/create.c | 10 ++++++++++
>   2 files changed, 28 insertions(+)

[...]

> diff --git a/block/create.c b/block/create.c
> index 89812669df..0167118579 100644
> --- a/block/create.c
> +++ b/block/create.c
> @@ -42,6 +42,16 @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
>       BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
>       int ret;
>   
> +    /*
> +     * Currently there is nothing preventing this
> +     * function from being called in an iothread context.
> +     * However, since it will crash anyways because of the
> +     * aiocontext lock not taken, we might as well make it
> +     * crash with a more meaningful error, by checking that
> +     * we are in the main loop
> +     */
> +    assert(qemu_in_main_thread());

Mostly agreed.  This function is always run in the main loop right now, 
so this assertion will never fail.

But that’s the “mostly”: Adding this assertion won’t give a more 
meaningful error, because the problem still remains that block drivers 
do not error out when encountering (or correctly handle) BDSs in 
non-main contexts, and so it remains a “qemu_mutex_unlock_impl: 
Operation not permitted”.

Not trying to say that that’s your problem.  It’s pre-existing, and this 
assertion is good.  Just wanting to clarify something about the comment 
that seemed unclear to me (in that I found it implied that the 
qemu_mutex_unlock_impl error would be replaced by this assertion failing).

Hanna

> +
>       job_progress_set_remaining(&s->common, 1);
>       ret = s->drv->bdrv_co_create(s->opts, errp);
>       job_progress_update(&s->common, 1);