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
