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

Commit Message

Daniel Henrique Barboza Sept. 3, 2019, 1:57 p.m. UTC

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é <[e-mail]>
Signed-off-by: Daniel Henrique Barboza <[e-mail]>
---
 block.c               | 73 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 74 insertions(+)

Comments

Kevin Wolf Oct. 18, 2019, 12:37 p.m. UTC | #1

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é <[e-mail]>
> Signed-off-by: Daniel Henrique Barboza <[e-mail]>

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

Patch

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;
+        goto out;
+    }
+
+    if (!bs->drv) {
+        error_setg(errp, "File '%s' has unknown format", bs->filename);
+        ret = -ENOMEDIUM;
+        goto out;
+    }
+
+    if (!bs->drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image delete",
+                   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);
+        }
+    }
+
+    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;
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..00fe8d6534 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -374,6 +374,7 @@  bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
 int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+int bdrv_delete_file(BlockDriverState *bs, Error **errp);
 
 
 typedef struct BdrvCheckResult {