Re: [PATCH v7 2/4] block.c: adding bdrv_delete_file

[Kevin Wolf <kwolf@redhat.com>]

Am 03.09.2019 um 15:57 hat Daniel Henrique Barboza geschrieben:
> Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
> can be used in a way similar of the existing bdrv_create_file to
> to clean up a created file.
> 
> The logic is also similar to what is already done in bdrv_create_file:
> a qemu_coroutine is created if needed, a specialized function
> bdrv_delete_co_entry is used to call the bdrv_co_delete_file
> co-routine of the driver, if the driver implements it.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

The only caller of bdrv_delete_file() is in coroutine context, so I
think this patch can be made much simpler by turning it into a pure
coroutine function bdrv_co_delete_file().

> diff --git a/block.c b/block.c
> index 874a29a983..250c69ca7a 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,79 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>      return ret;
>  }
>  
> +typedef struct DeleteCo {
> +    BlockDriver *drv;
> +    BlockDriverState *bs;
> +    int ret;
> +    Error *err;
> +} DeleteCo;
> +
> +static void coroutine_fn bdrv_delete_co_entry(void *opaque)
> +{
> +    Error *local_err = NULL;
> +    DeleteCo *dco = opaque;
> +    BlockDriver *drv = dco->bs->drv;
> +
> +    assert(dco->bs);
> +
> +    dco->ret = drv->bdrv_co_delete_file(dco->bs, &local_err);
> +    error_propagate(&dco->err, local_err);
> +}
> +
> +int bdrv_delete_file(BlockDriverState *bs, Error **errp)
> +{
> +    DeleteCo dco = {
> +        .bs = bs,
> +        .ret = NOT_DONE,
> +        .err = NULL,
> +    };
> +    Coroutine *co;
> +    int ret;
> +
> +    if (!bs) {
> +        error_setg(errp, "Could not open image '%s' for erasing",
> +                   bs->filename);
> +        ret = -1;

For a function returning 0/-errno, -1 is not a good return code because
it could be any error (or an undefined one). On Linux, this happens to
be -EPERM.

> +        goto out;
> +    }

We're not trying to open it here, so the error message is odd.

I think the caller should make sure that bs != NULL.

> +    if (!bs->drv) {
> +        error_setg(errp, "File '%s' has unknown format", bs->filename);
> +        ret = -ENOMEDIUM;
> +        goto out;
> +    }

bs->drv means that file isn't open. It has nothing to do with an unknown
format. Maybe you can combine both cases into one with an error message
"block node is not opened".

> +    if (!bs->drv->bdrv_co_delete_file) {
> +        error_setg(errp, "Driver '%s' does not support image delete",

s/delete/deletion/

> +                   bs->drv->format_name);
> +        ret = -ENOTSUP;
> +        goto out;
> +    }
> +
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        bdrv_delete_co_entry(&dco);
> +    } else {
> +        co = qemu_coroutine_create(bdrv_delete_co_entry, &dco);
> +        qemu_coroutine_enter(co);
> +        while (dco.ret == NOT_DONE) {
> +            aio_poll(qemu_get_aio_context(), true);
> +        }
> +    }

We don't really want to have this kind of different behaviour for
coroutine and non-coroutine contexts. It only exists for compatibility
reasons in other places (when we already had callers that didn't know
whether they were run inside a coroutine or not).

With a bdrv_co_delete_file(), it will go away.

> +
> +    ret = dco.ret;
> +    if (ret < 0) {
> +        if (dco.err) {
> +            error_propagate(errp, dco.err);
> +        } else {
> +            error_setg_errno(errp, -ret, "Could not delete image");
> +        }
> +    }
> +
> +out:
> +    return ret;

No cleanup code, so all "ret = ...; goto out;" instances above could be
replaced with a direct return.

Kevin