[Qemu-devel] [PATCH v1 0/1] qemu-img: add new function to remove bitmap in image

[Qemu-devel] [PATCH v1 0/1] qemu-img: add new function to remove bitmap in image

[Ma Haocong]

Hello,

In our scene, we need to delete dirty-bitmap created by using qmp command 
'block-dirty-bitmap-add'. we can use qmp command 'block-dirty-bitmap-remove'
to remove bitmap. Then I think that we should add a new function in qemu-img
to do the same work. 

The command format is: qemu-img removebmp file-name bitmap-name

I test the function by using qmp command to create a named dirty-bitmap in
qcow2 image, then shutdown the vm and using qemu-img to remove it. Then I
test to remove a bitmap with 'IN_USE' flags in qcow2 file, and it works.
I alse test to do the same job to the qcow2 image encrypted by luks.

Please help to review. Thanks.

mahaocong (1):
  Add new command 'removebmp' to remove bitmap in qcow2 file. The format
    is: qemu-img removebmp qcow2-file-name bitmap-name

 qemu-img-cmds.hx |   6 +++
 qemu-img.c       | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

-- 
2.14.1

[Qemu-devel] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Ma Haocong]

Signed-off-by: Ma Haocong <mahaocong_work@163.com>
---
 qemu-img-cmds.hx |   6 +++
 qemu-img.c       | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1526f327a5..cc397b64e4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -97,6 +97,12 @@ STEXI
 @item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ | -]@var{size}
 ETEXI
 
+DEF("removebmp", img_removebmp,
+    "removebmp [--object objectdef] [--image-opts] [-q] [-f fmt] filename dirtybitmap")
+STEXI
+@item removebmp [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] @var{filename} @var{dirtybitmap}
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..fdafb4a131 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -835,6 +835,125 @@ fail:
     return ret;
 }
 
+/*
+ * Remove a named dirty bitmap in image.
+ * This command should be used with no other QEMU process
+ * open the image at the same time. Otherwise it may be
+ * croputed the bitmap even the image.
+ */
+static int img_removebmp(int argc, char **argv)
+{
+    int               c, ret;
+    const char       *filename, *fmt, *bitmapname;
+    bool              quiet = false;
+    BlockBackend     *blk;
+    BlockDriverState *bs;
+    BdrvDirtyBitmap  *bitmap;
+    Error            *local_err = NULL;
+    bool image_opts = false;
+    fmt = NULL;
+
+    for (;;) {
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"object", required_argument, 0, OPTION_OBJECT},
+            {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, ":hf:q",
+                        long_options, &option_index);
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
+        case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
+            help();
+            break;
+        case 'f':
+            fmt = optarg;
+            break;
+        case 'q':
+            quiet = true;
+            break;
+        case OPTION_OBJECT: {
+            QemuOpts *opts;
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                return 1;
+            }
+        }   break;
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
+        }
+    }
+
+    if (optind != argc - 2) {
+        error_exit("Expecting two parameters");
+    }
+    filename = argv[optind++];
+    bitmapname = argv[optind++];
+
+    if (qemu_opts_foreach(&qemu_object_opts,
+                          user_creatable_add_opts_foreach,
+                          NULL, NULL)) {
+        return 1;
+    }
+
+    blk = img_open(image_opts, filename, fmt,
+                   BDRV_O_RDWR, false, quiet, false);
+    if (!blk) {
+        ret = -1;
+        goto out;
+    }
+    bs = blk_bs(blk);
+    if (!bs) {
+        ret = -1;
+        goto out;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
+
+    /*
+     * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. the
+     * qemu thread is corrupted and the 'IN_USE' flag is not be cleared),
+     * so the result of bdrv_find_dirty_bitmap is null. In this case,
+     * we delete bitmap in qcow2 file directly.
+    */
+    if (!bitmap) {
+        bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+        if (local_err != NULL) {
+            ret = -1;
+            goto out;
+        }
+    } else {
+        if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+            bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+            if (local_err != NULL) {
+                ret = -1;
+                goto out;
+            }
+        }
+        bdrv_release_dirty_bitmap(bs, bitmap);
+    }
+
+out:
+    blk_unref(blk);
+    if (ret) {
+        return 1;
+    }
+    return 0;
+}
+
 typedef struct CommonBlockJobCBInfo {
     BlockDriverState *bs;
     Error **errp;
-- 
2.14.1

Re: [Qemu-devel] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 3:37 AM, Ma Haocong wrote:
> Signed-off-by: Ma Haocong <mahaocong_work@163.com>
> ---
>   qemu-img-cmds.hx |   6 +++
>   qemu-img.c       | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 125 insertions(+)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 1526f327a5..cc397b64e4 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -97,6 +97,12 @@ STEXI
>   @item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ | -]@var{size}
>   ETEXI
>   
> +DEF("removebmp", img_removebmp,

Not alphabetical - if we keep this name (which I'm already questioning), 
this should come prior to 'resize'.

> +    "removebmp [--object objectdef] [--image-opts] [-q] [-f fmt] filename dirtybitmap")
> +STEXI
> +@item removebmp [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] @var{filename} @var{dirtybitmap}
> +ETEXI
> +
>   STEXI
>   @end table
>   ETEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index b12f4cd19b..fdafb4a131 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -835,6 +835,125 @@ fail:
>       return ret;
>   }
>   
> +/*
> + * Remove a named dirty bitmap in image.
> + * This command should be used with no other QEMU process
> + * open the image at the same time. Otherwise it may be
> + * croputed the bitmap even the image.

s/be croputed/corrupt/
s/even/or even/

However, the last sentence is not adding anything useful - we already 
have image locking that should make it impossible to attempt to remove a 
bitmap while some other qemu process has the image open.

> + */
> +static int img_removebmp(int argc, char **argv)
> +{
> +    int               c, ret;
> +    const char       *filename, *fmt, *bitmapname;
> +    bool              quiet = false;
> +    BlockBackend     *blk;
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap  *bitmap;
> +    Error            *local_err = NULL;
> +    bool image_opts = false;
> +    fmt = NULL;
> +
> +    for (;;) {
> +        int option_index = 0;
> +        static const struct option long_options[] = {
> +            {"help", no_argument, 0, 'h'},
> +            {"format", required_argument, 0, 'f'},
> +            {"object", required_argument, 0, OPTION_OBJECT},
> +            {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> +            {0, 0, 0, 0}
> +        };
> +        c = getopt_long(argc, argv, ":hf:q",
> +                        long_options, &option_index);

> +    bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
> +
> +    /*
> +     * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. the
> +     * qemu thread is corrupted and the 'IN_USE' flag is not be cleared),
> +     * so the result of bdrv_find_dirty_bitmap is null. In this case,
> +     * we delete bitmap in qcow2 file directly.
> +    */
> +    if (!bitmap) {
> +        bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
> +        if (local_err != NULL) {
> +            ret = -1;
> +            goto out;
> +        }
> +    } else {
> +        if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
> +            bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
> +            if (local_err != NULL) {
> +                ret = -1;
> +                goto out;
> +            }
> +        }
> +        bdrv_release_dirty_bitmap(bs, bitmap);
> +    }

Why aren't you calling bdrv_block_dirty_bitmap_remove()?  In general, 
HMP commands that are mere wrappers around counterpart QMP commands are 
easier to maintain, rather than open-coding the same work in two places.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 8:56 AM, Eric Blake wrote:

>> +    bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
>> +
>> +    /*
>> +     * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. 
>> the
>> +     * qemu thread is corrupted and the 'IN_USE' flag is not be 
>> cleared),
>> +     * so the result of bdrv_find_dirty_bitmap is null. In this case,
>> +     * we delete bitmap in qcow2 file directly.
>> +    */
>> +    if (!bitmap) {
>> +        bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
>> +        if (local_err != NULL) {
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +    } else {
>> +        if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
>> +            bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, 
>> &local_err);
>> +            if (local_err != NULL) {
>> +                ret = -1;
>> +                goto out;
>> +            }
>> +        }
>> +        bdrv_release_dirty_bitmap(bs, bitmap);
>> +    }
> 
> Why aren't you calling bdrv_block_dirty_bitmap_remove()?  In general, 

It helps if I ask my actual intended question: Why aren't you calling 
qmp_block_dirty_bitmap_remove()?

> HMP commands that are mere wrappers around counterpart QMP commands are 
> easier to maintain, rather than open-coding the same work in two places.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 0/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 3:37 AM, Ma Haocong wrote:
> Hello,
> 
> In our scene, we need to delete dirty-bitmap created by using qmp command
> 'block-dirty-bitmap-add'. we can use qmp command 'block-dirty-bitmap-remove'
> to remove bitmap. Then I think that we should add a new function in qemu-img
> to do the same work.
> 
> The command format is: qemu-img removebmp file-name bitmap-name

John was working on a more general 'qemu-img bitmap' command that did 
multiple operations from qemu-img, rather than just remove. That feels 
like a more extensible approach than adding a new command for every 
individual bitmap operation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org