Re: [PATCH v6 4/5] qemu-img: Add convert --bitmaps option

[Eric Blake <eblake@redhat.com>]

On 5/25/20 2:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.05.2020 22:21, Eric Blake wrote:
>> Make it easier to copy all the persistent bitmaps of (the top layer
>> of) a source image along with its guest-visible contents, by adding a
>> boolean flag for use with qemu-img convert.  This is basically
>> shorthand, as the same effect could be accomplished with a series of
>> 'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
>> commands, or by their corresponding QMP commands.
>>
>> Note that this command will fail in the same scenarios where 'qemu-img
>> measure' omits a 'bitmaps size:' line, namely, when either the source
>> or the destination lacks persistent bitmap support altogether.
>>
>> See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
>>
>> While touching this, clean up a couple coding issues spotted in the
>> same function: an extra blank line, and merging back-to-back 'if
>> (!skip_create)' blocks.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
> 

>> @@ -2573,6 +2632,13 @@ static int img_convert(int argc, char **argv)
>>       }
>>       out_bs = blk_bs(s.target);
>>
>> +    if (nbitmaps > 0 && 
>> !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
> 
> We will not fail, if target doesn't support bitmaps, source supports 
> them but has no bitmaps? Doesn't seem to be a problem, but a bit less 
> strict than you write in commit message.
> 
> So, maybe, s/nbitmaps > 0/bitmaps/

In fact, nbitmaps is not needed at all (it was useful in earlier 
iterations, but as the series has morphed, it is no longer buying me 
anything useful).

> 
>> +        error_report("Format driver '%s' does not support bitmaps",
>> +                     out_fmt);
> 
> Hmm seems, out_fmt may be NULL at this point, consider the path:
> const char *out_fmt = NULL
> ...
> [no -O option]
> --target-image-opts, so out_fmt doesn't default to "raw" but remains NULL
> ...
> 
> So, with s/out_fmt/out_bs->drv->format_name/ (and with or without 
> s/nbitmaps > 0/bitmaps/):
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Okay, I'm squashing this in, and adding your R-b.  Pull request coming 
shortly.

diff --git i/qemu-img.c w/qemu-img.c
index 8ecebe178890..d7e846e60742 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -2196,7 +2196,6 @@ static int img_convert(int argc, char **argv)
      bool force_share = false;
      bool explict_min_sparse = false;
      bool bitmaps = false;
-    size_t nbitmaps = 0;

      ImgConvertState s = (ImgConvertState) {
          /* Need at least 4k of zeros for sparse detection */
@@ -2565,10 +2564,8 @@ static int img_convert(int argc, char **argv)
          }
      }

-    /* Determine how many bitmaps need copying */
+    /* Determine if bitmaps need copying */
      if (bitmaps) {
-        BdrvDirtyBitmap *bm;
-
          if (s.src_num > 1) {
              error_report("Copying bitmaps only possible with single 
source");
              ret = -1;
@@ -2579,11 +2576,6 @@ static int img_convert(int argc, char **argv)
              ret = -1;
              goto out;
          }
-        FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
-            if (bdrv_dirty_bitmap_get_persistence(bm)) {
-                nbitmaps++;
-            }
-        }
      }

      /*
@@ -2632,9 +2624,9 @@ static int img_convert(int argc, char **argv)
      }
      out_bs = blk_bs(s.target);

-    if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
+    if (bitmaps && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
          error_report("Format driver '%s' does not support bitmaps",
-                     out_fmt);
+                     out_bs->drv->format_name);
          ret = -1;
          goto out;
      }
@@ -2700,7 +2692,7 @@ static int img_convert(int argc, char **argv)
      ret = convert_do_copy(&s);

      /* Now copy the bitmaps */
-    if (nbitmaps > 0 && ret == 0) {
+    if (bitmaps && ret == 0) {
          ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
      }



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