Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure

[Eric Blake]

On 5/20/20 6:00 PM, Nir Soffer wrote:

> > On the command-line side, 'qemu-img measure' gains a new --bitmaps
> > flag.  When present, the bitmap size is rolled into the two existing
> > measures (or errors if either the source image or destination format
> > lacks bitmaps); when absent, there is never an error (for
> > back-compat), but the output will instead include a new line item for
> > bitmaps (which you would have to manually add), with that line being
> > omitted in the same cases where passing --bitmaps would error.
>
> Supporting 2 ways to measure, one by specifying --bitmaps, and another
> by adding bitmaps key is not a good idea. We really need one way.
>
> Each one has advantages. adding --bitmaps flag is consistent with
> "qemu-img convert"
> and future extensions that may require  new flag, and adding "bitmaps"
> key is consistent
> with "qmeu-img info", showing bitmaps when they exist.
>
> Adding a "bitmaps" key has an advantage that we can use it to test if qemu-img
> supports measuring and copying bitmaps (since both features are expected to
> be delivered at the same time). So we can avoid checking --help learn about
> the capabilities.
>
> I'm ok with both options, can we have only one?

That was the crux of the conversation after v3, where we were trying to 
figure out what interface you actually needed.  I implemented both to 
show the difference, but if you want only one, then my preference is to 
delete the --bitmaps option and only expose the optional 'bitmaps size' 
field in all cases where bitmaps can be copied.

Here's the diff that would accomplish that:

diff --git i/docs/tools/qemu-img.rst w/docs/tools/qemu-img.rst
index 35050fc51070..69cd9a30373a 100644
--- i/docs/tools/qemu-img.rst
+++ w/docs/tools/qemu-img.rst
@@ -597,7 +597,7 @@ Command description:
   For more information, consult ``include/block/block.h`` in QEMU's
   source code.

-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] 
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l 
SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] 
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l 
SNAPSHOT_PARAM] FILENAME]

   Calculate the file size required for a new image.  This information
   can be used to size logical volumes or SAN LUNs appropriately for
@@ -634,10 +634,7 @@ Command description:
   copy bitmaps from a source image in addition to the guest-visible
   data; the line is omitted if either source or destination lacks
   bitmap support, or 0 if bitmaps are supported but there is nothing
-  to copy.  If the ``--bitmaps`` option is in use, the bitmap size is
-  instead folded into the required and fully-allocated size for
-  convenience, rather than being a separate line item; using the
-  option will raise an error if bitmaps are not supported.
+  to copy.

 .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l 
| -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME

diff --git i/qemu-img.c w/qemu-img.c
index 1494d8f5c409..8dc4cae2c274 100644
--- i/qemu-img.c
+++ w/qemu-img.c
@@ -5207,7 +5207,6 @@ static int img_measure(int argc, char **argv)
         {"output", required_argument, 0, OPTION_OUTPUT},
         {"size", required_argument, 0, OPTION_SIZE},
         {"force-share", no_argument, 0, 'U'},
-        {"bitmaps", no_argument, 0, OPTION_BITMAPS},
         {0, 0, 0, 0}
     };
     OutputFormat output_format = OFORMAT_HUMAN;
@@ -5224,7 +5223,6 @@ static int img_measure(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     QemuOptsList *create_opts = NULL;
     bool image_opts = false;
-    bool bitmaps = false;
     uint64_t img_size = UINT64_MAX;
     BlockMeasureInfo *info = NULL;
     Error *local_err = NULL;
@@ -5297,10 +5295,6 @@ static int img_measure(int argc, char **argv)
             img_size = (uint64_t)sval;
         }
         break;
-        case OPTION_BITMAPS:
-            bitmaps = true;
-            break;
-        }
     }

     if (qemu_opts_foreach(&qemu_object_opts,
@@ -5328,10 +5322,6 @@ static int img_measure(int argc, char **argv)
         error_report("Either --size N or one filename must be 
specified.");
         goto out;
     }
-    if (!filename && bitmaps) {
-        error_report("--bitmaps is only supported with a filename.");
-        goto out;
-    }

     if (filename) {
         in_blk = img_open(image_opts, filename, fmt, 0,
@@ -5387,18 +5377,6 @@ static int img_measure(int argc, char **argv)
         goto out;
     }

-    if (bitmaps) {
-        if (!info->has_bitmaps) {
-            error_report("no bitmaps measured, either source or 
destination "
-                         "format lacks bitmap support");
-            goto out;
-        } else {
-            info->required += info->bitmaps;
-            info->fully_allocated += info->bitmaps;
-            info->has_bitmaps = false;
-        }
-    }
-
     if (output_format == OFORMAT_HUMAN) {
         printf("required size: %" PRIu64 "\n", info->required);
         printf("fully allocated size: %" PRIu64 "\n", 
info->fully_allocated);
diff --git i/qemu-img-cmds.hx w/qemu-img-cmds.hx
index e9beb15e614e..10b910b67cf8 100644
--- i/qemu-img-cmds.hx
+++ w/qemu-img-cmds.hx
@@ -76,9 +76,9 @@ SRST
 ERST

 DEF("measure", img_measure,
-"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | 
[--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l 
snapshot_param] filename]")
+"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N | 
[--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
 SRST
-.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] 
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l 
SNAPSHOT_PARAM] FILENAME]
+.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS] 
[--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l 
SNAPSHOT_PARAM] FILENAME]
 ERST

 DEF("snapshot", img_snapshot,


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