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

[Eric Blake <eblake@redhat.com>]

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 <vsementsov@virtuozzo.com>

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