Re: [PATCH v4 6/9] qemu-img: Add bitmap sub-command

[Eric Blake]

On 5/18/20 6:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
> > Include actions for --add, --remove, --clear, --enable, --disable, and
> > --merge (note that --clear is a bit of fluff, because the same can be
> > accomplished by removing a bitmap and then adding a new one in its
> > place, but it matches what QMP commands exist).  Listing is omitted,
> > because it does not require a bitmap name and because it was already
> > possible with 'qemu-img info'.  A single command line can play one or
> > more bitmap commands in sequence on the same bitmap name (although all
> > added bitmaps share the same granularity, and and all merged bitmaps
> > come from the same source file).  Merge defaults to other bitmaps in
> > the primary image, but can also be told to merge bitmaps from a
> > distinct image.
> >
> > While this supports --image-opts for the file being modified, I did
> > not think it worth the extra complexity to support that for the source
> > file in a cross-file merges.  Likewise, I chose to have --merge only
> > take a single source rather than following the QMP support for
> > multiple merges in one go (although you can still use more than one
> > --merge in the command line); in part because qemu-img is offline and
> > therefore atomicity is not an issue.

> > +
> > +    blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
> > +                   false);
>
> fit in one line

That line would be exactly 80; I tend to wrap at 79 or earlier rather 
than exactly on 80.

> > +    if (!blk) {
> > +        goto out;
> > +    }
> > +    bs = blk_bs(blk);
> > +    if (src_filename) {
> > +        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> > +                       false);
>
> s/NULL/false/

D'oh.  And yet it still compiles.  Fixing.

> also, fit in one line

Yes, this one's shorter.  Fixing.


> > +
> > +        if (err) {
> > +            error_reportf_err(err, "Operation %s on bitmap %s failed",
>
> s/failed/failed: /
>
> > +                              op, bitmap);
> > +            ret = -1;
>
> dead assignment: you never set ret after first initialization to -1.

Fixing both.

> > +            goto out;
> > +        }
> > +        g_free(act);
> > +    }
> > +
> > +    ret = 0;
> > +
> > + out:
> > +    blk_unref(src);
> > +    blk_unref(blk);
> > +    qemu_opts_del(opts);
> > +    if (ret) {
> > +        return 1;
> > +    }
> > +    return 0;
>
> Hmm, as it's the only usage of ret, you may initialize it to 1 at 
> function start, and here just "return ret;"

Yep, done.

> > +DEF("bitmap", img_bitmap,
> > +    "bitmap (--merge SOURCE | --add | --remove | --clear | --enable | 
> > --disable)... [-b source_file [-F source_fmt]] [-g granularity] 
> > [--object objectdef] [--image-opts | -f fmt] filename bitmap")
> > +SRST
> > +.. option:: bitmap (--merge SOURCE | --add | --remove | --clear | 
> > --enable | --disable)... [-b SOURCE_FILE [-F SOURCE_FMT]] [-g 
> > GRANULARITY] [--object OBJECTDEF] [--image-opts | -f FMT] FILENAME BITMAP
>
> Not about this patch, but it's a pity that we have triple duplications 
> (triplications ?) of such lines..

Yes, it is annoying.  But as you say, it's a cleanup for another day, 
for someone who is interested.

> With at least s/NULL/false/ and s/failed/failed: / (or with all my tiny 
> suggestions):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Thanks; squashing in:

diff --git i/qemu-img.c w/qemu-img.c
index 8c99e68ba8aa..f940137cb0e5 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -4493,7 +4493,7 @@ typedef struct ImgBitmapAction {
 static int img_bitmap(int argc, char **argv)
 {
     Error *err = NULL;
-    int c, ret = -1;
+    int c, ret = 1;
     QemuOpts *opts = NULL;
     const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
     const char *filename, *bitmap;
@@ -4641,8 +4641,7 @@ static int img_bitmap(int argc, char **argv)
     }
     bs = blk_bs(blk);
     if (src_filename) {
-        src = img_open(NULL, src_filename, src_fmt, 0, false, false,
-                       false);
+        src = img_open(false, src_filename, src_fmt, 0, false, false, 
false);
         if (!src) {
             goto out;
         }
@@ -4695,9 +4694,8 @@ static int img_bitmap(int argc, char **argv)
         }

         if (err) {
-            error_reportf_err(err, "Operation %s on bitmap %s failed",
+            error_reportf_err(err, "Operation %s on bitmap %s failed: ",
                               op, bitmap);
-            ret = -1;
             goto out;
         }
         g_free(act);
@@ -4709,10 +4707,7 @@ static int img_bitmap(int argc, char **argv)
     blk_unref(src);
     blk_unref(blk);
     qemu_opts_del(opts);
-    if (ret) {
-        return 1;
-    }
-    return 0;
+    return ret;
 }

 #define C_BS      01

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