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

[Eric Blake]

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 <address@hidden>
> > ---

> > @@ -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 <address@hidden>

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