Re: [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not

[Kevin Wolf <kwolf@redhat.com>]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> 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>
> ---
>  block.c | 76 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 577639c7e0..c610a32e77 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,66 +528,66 @@ 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;
> +    char *filename_copy;
> +    GLOBAL_STATE_CODE();
> +    assert(*errp == NULL);

Your use of *errp in this function will crash if NULL is passed. You
need ERRP_GUARD() first before you can do this.

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

It's only preserved from the pre-patch state, but I don't think this is
necessary? We know that the string will stay around until the function
returns, and the parameter of drv->bdrv_co_create_opts is const char*,
so it must not be modified either.

Maybe worth a cleanup patch on top to just use filename directly?

> +    ret = drv->bdrv_co_create_opts(drv, filename_copy, opts, errp);
> +
> +    if (ret < 0 && !*errp) {
> +        error_setg_errno(errp, -ret, "Could not create image");
> +    }
> +
> +    g_free(filename_copy);
> +    return ret;
> +}

Kevin