On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> 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é <berra...@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
> ---
> block.c | 77 +++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> 2 files changed, 78 insertions(+)
>
> diff --git a/block.c b/block.c
> index cbd8da5f3b..1e20250627 100644
> --- a/block.c
> +++ b/block.c
> @@ -547,6 +547,83 @@ 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;
> +
> + assert(dco->bs);
> +
> + dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
> + error_propagate(&dco->err, local_err);
> +}
> +
> +int bdrv_delete_file(const char *filename, Error **errp)
> +{
> + BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
> + BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
> + DeleteCo dco = {
> + .drv = drv,
> + .bs = bs,
> + .ret = NOT_DONE,
> + .err = NULL,
> + };
> + Coroutine *co;
> + int ret;
> +
> + if (!drv) {
> + error_setg(errp, "File '%s' has unknown format", filename);
> + ret = -ENOENT;
> + goto out;
> + }
> +
I was going to say that ENOENT is a weird error here, but I see it used
for !drv a few other places in block.c too, alongside EINVAL and
ENOMEDIUM. ENOMEDIUM loks like the most popular.
> + if (!drv->bdrv_co_delete_file) {
> + error_setg(errp, "Driver '%s' does not support image delete",
> + drv->format_name);
> + ret = -ENOTSUP;
> + goto out;
> + }
> +
> + if (!bs) {
> + error_setg(errp, "Could not open image '%s' for erasing",
> + filename);
> + ret = 1;
Please keep all errors negative (or at least consistent within a function).
I'm also wondering if we want a version of delete that doesn't try to
open a file directly -- i.e. a version that exists like this:
bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
That simply dispatches based on bs->drv to the correct routine.
Then, you are free to have bdrv_delete_file handle the open (and let the
opening figure out what driver it needs), and just hand off the bds to
bdrv_co_delete_file.
I'm not the authority for block.c, though, so maaaybe I'm giving you bad
advice here. Kevin's away on PTO for a bit and gave you advice most
recently, so I might try to gently ask him for more feedback next week.
> + 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:
> + bdrv_unref(bs);
> + 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 50a07c1c33..5e83532364 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -369,6 +369,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(const char *filename, Error **errp);
>
>
> typedef struct BdrvCheckResult {
>
