Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not

[Vladimir Sementsov-Ogievskiy]

On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> Call two different functions depending on whether bdrv_create
> is in coroutine or not, following the same pattern as
> generated_co_wrapper functions.
>
> This allows to also call the coroutine function directly,
> without using CreateCo or relying in bdrv_create().
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 71 +++++++++++++++++++++++++++++----------------------------
>   1 file changed, 36 insertions(+), 35 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9d51e7b6e5..2cf50b37c4 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,63 +528,64 @@ typedef struct CreateCo {
>       Error *err;
>   } CreateCo;
>   
> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> +                                       QemuOpts *opts, Error **errp)
>   {
> -    Error *local_err = NULL;
>       int ret;
> +    GLOBAL_STATE_CODE();
> +    ERRP_GUARD();
> +    assert(*errp == NULL);
> +    assert(drv);

Why we need these two assertions? These are general assumptions, and we don't 
assert it in all functions. Dereference of NULL will crash not worse than 
assertion. I'd drop them.

> +
> +    if (!drv->bdrv_co_create_opts) {
> +        error_setg(errp, "Driver '%s' does not support image creation",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }
> +
> +    ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>   

and this empty line, looks accidental.

Offtopic: hope one day we fix *open* functions to always set errp on error 
paths.

> +    if (ret < 0 && !*errp) {
> +        error_setg_errno(errp, -ret, "Could not create image");
> +    }
> +
> +    return ret;
> +}
> +
> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +{
>       CreateCo *cco = opaque;
> -    assert(cco->drv);
>       GLOBAL_STATE_CODE();
>   
> -    ret = cco->drv->bdrv_co_create_opts(cco->drv,
> -                                        cco->filename, cco->opts, &local_err);
> -    error_propagate(&cco->err, local_err);
> -    cco->ret = ret;
> +    cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->err);

We need aio_wait_kick() call here, like in other co_entry() functions. 
Otherwise we may stuck in aio_poll()

with it:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Hmm actually, we can simply merge this patch into patch 13 (and move 08 to be 
after 13). Why to refactor bdrv_create twice?


--
Best regards,
Vladimir