[PATCH v3 0/4] Additional parameters for qemu_img map

[PATCH v3 0/4] Additional parameters for qemu_img map

[Eyal Moscovici]

Hi,

The following series adds two parameters to qemu-img map:
1. start-offset: mapping starting offset.
2. max-length: the length of the mapping.

These parameters proved useful when mapping large disk spread across
long store file chains. It allows us to bound the execution time of each
qemu-img map execution as well as recover from failed mapping
operations. In addition the map operation can divided to
multiple independent tasks.

V3 changes:
1. Add cvtnum_full and made cvtnum a wrapper function.
2. Keep the original boundaries checks.
3. Tone down error messages.

V2 changes:
1. Add error reporting to cvtnum.
2. Add image length validation in img_map.
3. Rebase over QEMU 5.0.

Eyal Moscovici (1):
  qemu_img: add cvtnum_full to print error reports

 qemu-img.c                 | 76 +++++++++++++++++---------------------
 tests/qemu-iotests/049.out |  8 ++--
 2 files changed, 38 insertions(+), 46 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH v3 1/1] qemu_img: add cvtnum_full to print error reports

[Eyal Moscovici]

All calls to cvtnum check the return value and print the same error message more
or less. And so error reporting moved to cvtnum_full to reduce code
duplication and provide a single error message. Additionally, cvtnum now wraps
cvtnum_full with the existing default range of 0 to MAX_INT64.

Acked-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 qemu-img.c                 | 76 +++++++++++++++++---------------------
 tests/qemu-iotests/049.out |  8 ++--
 2 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..a4ce35abc5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -470,19 +470,31 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
     return 0;
 }
 
-static int64_t cvtnum(const char *s)
+static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
+                           int64_t max)
 {
     int err;
-    uint64_t value;
-
-    err = qemu_strtosz(s, NULL, &value);
-    if (err < 0) {
+    uint64_t res;
+
+    err = qemu_strtosz(value, NULL, &res);
+    if (err < 0 && err != -ERANGE) {
+        error_report("Invalid %s specified. You may use "
+                     "k, M, G, T, P or E suffixes for ", name);
+        error_report("kilobytes, megabytes, gigabytes, terabytes, "
+                     "petabytes and exabytes.");
         return err;
     }
-    if (value > INT64_MAX) {
+    if (err == -ERANGE || res > max || res < min) {
+        error_report("Invalid %s specified. Must be between %ld bytes "
+                     "to %ld bytes.", name, min, max);
         return -ERANGE;
     }
-    return value;
+    return res;
+}
+
+static int64_t cvtnum(const char *name, const char *value)
+{
+    return cvtnum_full(name, value, 0, INT64_MAX);
 }
 
 static int img_create(int argc, char **argv)
@@ -572,16 +584,8 @@ static int img_create(int argc, char **argv)
     if (optind < argc) {
         int64_t sval;
 
-        sval = cvtnum(argv[optind++]);
+        sval = cvtnum("image size", argv[optind++]);
         if (sval < 0) {
-            if (sval == -ERANGE) {
-                error_report("Image size must be less than 8 EiB!");
-            } else {
-                error_report("Invalid image size specified! You may use k, M, "
-                      "G, T, P or E suffixes for ");
-                error_report("kilobytes, megabytes, gigabytes, terabytes, "
-                             "petabytes and exabytes.");
-            }
             goto fail;
         }
         img_size = (uint64_t)sval;
@@ -2187,8 +2191,10 @@ static int img_convert(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
-            if (sval < 0 || !QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
+            sval = cvtnum("buffer size for sparse output", optarg);
+            if (sval < 0) {
+                goto fail_getopt;
+            } else if (!QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
                 sval / BDRV_SECTOR_SIZE > MAX_BUF_SECTORS) {
                 error_report("Invalid buffer size for sparse output specified. "
                     "Valid sizes are multiples of %llu up to %llu. Select "
@@ -4291,9 +4297,8 @@ static int img_bench(int argc, char **argv)
             break;
         case 'o':
         {
-            offset = cvtnum(optarg);
+            offset = cvtnum("offset", optarg);
             if (offset < 0) {
-                error_report("Invalid offset specified");
                 return 1;
             }
             break;
@@ -4306,9 +4311,8 @@ static int img_bench(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
-                error_report("Invalid buffer size specified");
+            sval = cvtnum_full("buffer size", optarg, 0, INT_MAX);
+            if (sval < 0) {
                 return 1;
             }
 
@@ -4319,9 +4323,8 @@ static int img_bench(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
-                error_report("Invalid step size specified");
+            sval = cvtnum_full("step_size", optarg, 0, INT_MAX);
+            if (sval < 0) {
                 return 1;
             }
 
@@ -4491,10 +4494,9 @@ static int img_dd_bs(const char *arg,
 {
     int64_t res;
 
-    res = cvtnum(arg);
+    res = cvtnum_full("bs", arg, 1, INT_MAX);
 
-    if (res <= 0 || res > INT_MAX) {
-        error_report("invalid number: '%s'", arg);
+    if (res < 0) {
         return 1;
     }
     in->bsz = out->bsz = res;
@@ -4506,10 +4508,9 @@ static int img_dd_count(const char *arg,
                         struct DdIo *in, struct DdIo *out,
                         struct DdInfo *dd)
 {
-    dd->count = cvtnum(arg);
+    dd->count = cvtnum("count", arg);
 
     if (dd->count < 0) {
-        error_report("invalid number: '%s'", arg);
         return 1;
     }
 
@@ -4538,10 +4539,9 @@ static int img_dd_skip(const char *arg,
                        struct DdIo *in, struct DdIo *out,
                        struct DdInfo *dd)
 {
-    in->offset = cvtnum(arg);
+    in->offset = cvtnum("skip", arg);
 
     if (in->offset < 0) {
-        error_report("invalid number: '%s'", arg);
         return 1;
     }
 
@@ -4923,16 +4923,8 @@ static int img_measure(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
+            sval = cvtnum("image size", optarg);
             if (sval < 0) {
-                if (sval == -ERANGE) {
-                    error_report("Image size must be less than 8 EiB!");
-                } else {
-                    error_report("Invalid image size specified! You may use "
-                                 "k, M, G, T, P or E suffixes for ");
-                    error_report("kilobytes, megabytes, gigabytes, terabytes, "
-                                 "petabytes and exabytes.");
-                }
                 goto out;
             }
             img_size = (uint64_t)sval;
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index affa55b341..e00d311180 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -92,19 +92,19 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1649267441664 cluster_size=65536 l
 == 3. Invalid sizes ==
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified. Must be between 0 bytes to 9223372036854775807 bytes.
 
 qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2
 qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size'
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k
-qemu-img: Image size must be less than 8 EiB!
+qemu-img: Invalid image size specified. Must be between 0 bytes to 9223372036854775807 bytes.
 
 qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2
 qemu-img: TEST_DIR/t.qcow2: Value '-1k' is out of range for parameter 'size'
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
-qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
 qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 
 qemu-img create -f qcow2 -o size=1kilobyte TEST_DIR/t.qcow2
@@ -113,7 +113,7 @@ Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
 and exabytes, respectively.
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- foobar
-qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
 qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 
 qemu-img create -f qcow2 -o size=foobar TEST_DIR/t.qcow2
-- 
2.17.2 (Apple Git-113)

Re: [PATCH v3 1/1] qemu_img: add cvtnum_full to print error reports

[Eric Blake]

On 5/13/20 8:36 AM, Eyal Moscovici wrote:
> All calls to cvtnum check the return value and print the same error message more
> or less. And so error reporting moved to cvtnum_full to reduce code
> duplication and provide a single error message. Additionally, cvtnum now wraps
> cvtnum_full with the existing default range of 0 to MAX_INT64.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---

> -static int64_t cvtnum(const char *s)
> +static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
> +                           int64_t max)
>   {
>       int err;
> -    uint64_t value;
> -
> -    err = qemu_strtosz(s, NULL, &value);
> -    if (err < 0) {
> +    uint64_t res;
> +
> +    err = qemu_strtosz(value, NULL, &res);
> +    if (err < 0 && err != -ERANGE) {
> +        error_report("Invalid %s specified. You may use "
> +                     "k, M, G, T, P or E suffixes for ", name);
> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                     "petabytes and exabytes.");

The new common message is in terms of bytes, even though not all callers 
are specifically related to bytes.  I don't think it's a show-stopper, 
though, merely a quality of error message, and I don't have any ideas 
for how to improve it.

> @@ -4506,10 +4508,9 @@ static int img_dd_count(const char *arg,
>                           struct DdIo *in, struct DdIo *out,
>                           struct DdInfo *dd)
>   {
> -    dd->count = cvtnum(arg);
> +    dd->count = cvtnum("count", arg);
>   
>       if (dd->count < 0) {
> -        error_report("invalid number: '%s'", arg);

For example, the count argument to dd is not really about bytes, but 
repetitions.  So the error message is now more informative (what 
suffixes you can use) but also less accurate ("invalid number" was vague 
but at least correct).

I think the common code is worth the corner case regressions, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v3 1/1] qemu_img: add cvtnum_full to print error reports

[Eric Blake]

On 5/13/20 8:36 AM, Eyal Moscovici wrote:
> All calls to cvtnum check the return value and print the same error message more
> or less. And so error reporting moved to cvtnum_full to reduce code
> duplication and provide a single error message. Additionally, cvtnum now wraps
> cvtnum_full with the existing default range of 0 to MAX_INT64.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---

> -static int64_t cvtnum(const char *s)
> +static int64_t cvtnum_full(const char *name, const char *value, int64_t min,
> +                           int64_t max)
>   {
>       int err;
> -    uint64_t value;
> -
> -    err = qemu_strtosz(s, NULL, &value);
> -    if (err < 0) {
> +    uint64_t res;
> +
> +    err = qemu_strtosz(value, NULL, &res);
> +    if (err < 0 && err != -ERANGE) {
> +        error_report("Invalid %s specified. You may use "
> +                     "k, M, G, T, P or E suffixes for ", name);
> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                     "petabytes and exabytes.");

Consecutive error_report() calls each output a newline, which means your 
new output includes a trailing space.

> @@ -572,16 +584,8 @@ static int img_create(int argc, char **argv)
>       if (optind < argc) {
>           int64_t sval;
>   
> -        sval = cvtnum(argv[optind++]);
> +        sval = cvtnum("image size", argv[optind++]);
>           if (sval < 0) {
> -            if (sval == -ERANGE) {
> -                error_report("Image size must be less than 8 EiB!");
> -            } else {
> -                error_report("Invalid image size specified! You may use k, M, "
> -                      "G, T, P or E suffixes for ");
> -                error_report("kilobytes, megabytes, gigabytes, terabytes, "
> -                             "petabytes and exabytes.");
> -            }

True, that's what some of the old code was doing, but...

> +++ b/tests/qemu-iotests/049.out

>   
>   qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
> -qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
> +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for

where it gets hairy is that our iotests _intentionally_ strip trailing 
space before comparing to expected output, because it is such a pain to 
commit files with trailing spaces into the repository.  We're better off 
making the expected output precisely match what qemu-img actually 
outputs, which means using this as an opportunity to fix qemu-img to not 
output trailing space in the first place.

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

[PATCH v3 2/4] qemu-img: validate image length in img_map

[Eyal Moscovici]

The code handles this case correctly we merely skip the loop. However it
is probably best to return an explicit error.

Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 qemu-img.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index cc2e4a3799..23e90a99e1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3091,6 +3091,11 @@ static int img_map(int argc, char **argv)
     }
 
     length = blk_getlength(blk);
+    if (length < 0) {
+        error_report("Failed to get size for '%s'", filename);
+        return 1;
+    }
+
     while (curr.start + curr.length < length) {
         int64_t offset = curr.start + curr.length;
         int64_t n;
-- 
2.17.2 (Apple Git-113)

Re: [PATCH v3 2/4] qemu-img: validate image length in img_map

[Eric Blake]

On 5/13/20 8:36 AM, Eyal Moscovici wrote:
> The code handles this case correctly we merely skip the loop. However it

Grammar suggestion: s/correctly we/correctly: we/

> is probably best to return an explicit error.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

R-b still stands.

> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>   qemu-img.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index cc2e4a3799..23e90a99e1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3091,6 +3091,11 @@ static int img_map(int argc, char **argv)
>       }
>   
>       length = blk_getlength(blk);
> +    if (length < 0) {
> +        error_report("Failed to get size for '%s'", filename);
> +        return 1;
> +    }
> +
>       while (curr.start + curr.length < length) {
>           int64_t offset = curr.start + curr.length;
>           int64_t n;
> 

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

[PATCH v3 3/4] qemu-img: refactor dump_map_entry JSON format output

[Eyal Moscovici]

Previously dump_map_entry identified whether we need to start a new JSON
array based on whether start address == 0. In this refactor we remove
this assumption as in following patches we will allow map to start from
an arbitrary position.

Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 qemu-img.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 23e90a99e1..80340cb218 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2901,9 +2901,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         break;
     case OFORMAT_JSON:
-        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+        printf("{ \"start\": %"PRId64", \"length\": %"PRId64","
                " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
-               (e->start == 0 ? "[" : ",\n"),
                e->start, e->length, e->depth,
                e->zero ? "true" : "false",
                e->data ? "true" : "false");
@@ -2912,8 +2911,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         putchar('}');
 
-        if (!next) {
-            printf("]\n");
+        if (next) {
+            puts(",");
         }
         break;
     }
@@ -3088,6 +3087,8 @@ static int img_map(int argc, char **argv)
 
     if (output_format == OFORMAT_HUMAN) {
         printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");
+    } else if (output_format == OFORMAT_JSON) {
+        putchar('[');
     }
 
     length = blk_getlength(blk);
@@ -3124,6 +3125,9 @@ static int img_map(int argc, char **argv)
     }
 
     ret = dump_map_entry(output_format, &curr, NULL);
+    if (output_format == OFORMAT_JSON) {
+        puts("]");
+    }
 
 out:
     blk_unref(blk);
-- 
2.17.2 (Apple Git-113)

[PATCH v3 4/4] qemu-img: Add --start-offset and --max-length to map

[Eyal Moscovici]

The mapping operation of large disks especially ones stored over a
long chain of QCOW2 files can take a long time to finish.
Additionally when mapping fails there was no way recover by
restarting the mapping from the failed location.

The new options, --start-offset and --max-length allows the user to
divide these type of map operations into shorter independent tasks.

Reviewed-by: Eric Blake <eblake@redhat.com>
Acked-by: Mark Kanda <mark.kanda@oracle.com>
Co-developed-by: Yoav Elnekave <yoav.elnekave@oracle.com>
Signed-off-by: Yoav Elnekave <yoav.elnekave@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 docs/tools/qemu-img.rst |  2 +-
 qemu-img-cmds.hx        |  4 ++--
 qemu-img.c              | 22 +++++++++++++++++++++-
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76..f4ffe528ea 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -519,7 +519,7 @@ Command description:
     ``ImageInfoSpecific*`` QAPI object (e.g. ``ImageInfoSpecificQCow2``
     for qcow2 images).
 
-.. option:: map [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT] [-U] FILENAME
+.. option:: map [--object OBJECTDEF] [--image-opts] [-f FMT] [--start-offset=OFFSET] [--max-length=LEN] [--output=OFMT] [-U] FILENAME
 
   Dump the metadata of image *FILENAME* and its backing file chain.
   In particular, this commands dumps the allocation state of every sector
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index c9c54de1df..35f832816f 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -63,9 +63,9 @@ SRST
 ERST
 
 DEF("map", img_map,
-    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [--start-offset=offset] [--max-length=len] [--output=ofmt] [-U] filename")
 SRST
-.. option:: map [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT] [-U] FILENAME
+.. option:: map [--object OBJECTDEF] [--image-opts] [-f FMT] [--start-offset=OFFSET] [--max-length=LEN] [--output=OFMT] [-U] FILENAME
 ERST
 
 DEF("measure", img_measure,
diff --git a/qemu-img.c b/qemu-img.c
index 80340cb218..bce7b49799 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3008,6 +3008,8 @@ static int img_map(int argc, char **argv)
     int ret = 0;
     bool image_opts = false;
     bool force_share = false;
+    int64_t start_offset = 0;
+    int64_t max_length = -1;
 
     fmt = NULL;
     output = NULL;
@@ -3020,9 +3022,11 @@ static int img_map(int argc, char **argv)
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {"force-share", no_argument, 0, 'U'},
+            {"start-offset", required_argument, 0, 's'},
+            {"max-length", required_argument, 0, 'l'},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":f:hU",
+        c = getopt_long(argc, argv, ":f:s:l:hU",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -3046,6 +3050,18 @@ static int img_map(int argc, char **argv)
         case OPTION_OUTPUT:
             output = optarg;
             break;
+        case 's':
+            start_offset = cvtnum("start offset", optarg);
+            if (start_offset < 0) {
+                return 1;
+            }
+            break;
+        case 'l':
+            max_length = cvtnum("max length", optarg);
+            if (max_length < 0) {
+                return 1;
+            }
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3096,7 +3112,11 @@ static int img_map(int argc, char **argv)
         error_report("Failed to get size for '%s'", filename);
         return 1;
     }
+    if (max_length != -1) {
+        length = MIN(start_offset + max_length, length);
+    }
 
+    curr.start = start_offset;
     while (curr.start + curr.length < length) {
         int64_t offset = curr.start + curr.length;
         int64_t n;
-- 
2.17.2 (Apple Git-113)

Re: [PATCH v3 0/4] Additional parameters for qemu_img map

[Eric Blake]

On 5/13/20 8:36 AM, Eyal Moscovici wrote:
> Hi,
> 
> The following series adds two parameters to qemu-img map:
> 1. start-offset: mapping starting offset.
> 2. max-length: the length of the mapping.
> 
> These parameters proved useful when mapping large disk spread across
> long store file chains. It allows us to bound the execution time of each
> qemu-img map execution as well as recover from failed mapping
> operations. In addition the map operation can divided to
> multiple independent tasks.
> 
> V3 changes:
> 1. Add cvtnum_full and made cvtnum a wrapper function.
> 2. Keep the original boundaries checks.
> 3. Tone down error messages.

While this does not directly touch NBD code, I find it quite handy for 
my tests of incremental backups over NBD (since I frequently use 
x-dirty-bitmap coupled with qemu-img map to read bitmaps, and subsetting 
the output is indeed nice), so I'll queue this through my NBD tree.  It 
may be another week or so before I send a pull request including this 
and other collected patches.

Congratulations on your first qemu contribution!

>  qemu-img.c                 | 76 +++++++++++++++++---------------------
>  tests/qemu-iotests/049.out |  8 ++--
>  2 files changed, 38 insertions(+), 46 deletions(-)

This series diffstat is off; later in the series, in 4/4, I see:

>  docs/tools/qemu-img.rst |  2 +-
>  qemu-img-cmds.hx        |  4 ++--
>  qemu-img.c              | 22 +++++++++++++++++++++-
>  3 files changed, 24 insertions(+), 4 deletions(-)

What I don't see is any iotest coverage of the new options, to ensure 
they don't regress.  Either a new iotest, or an enhancement to an 
existing iotest.  If you feel up to the task, post a 5/4 patch; if not, 
I'll probably enhance 223 (my x-dirty-bitmap reading code mentioned above).

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

Re: [PATCH v3 0/4] Additional parameters for qemu_img map

[Eyal Moscovici]

On 13/05/2020 20:49, Eric Blake wrote:
> On 5/13/20 8:36 AM, Eyal Moscovici wrote:
>> Hi,
>>
>> The following series adds two parameters to qemu-img map:
>> 1. start-offset: mapping starting offset.
>> 2. max-length: the length of the mapping.
>>
>> These parameters proved useful when mapping large disk spread across
>> long store file chains. It allows us to bound the execution time of each
>> qemu-img map execution as well as recover from failed mapping
>> operations. In addition the map operation can divided to
>> multiple independent tasks.
>>
>> V3 changes:
>> 1. Add cvtnum_full and made cvtnum a wrapper function.
>> 2. Keep the original boundaries checks.
>> 3. Tone down error messages.
>
> While this does not directly touch NBD code, I find it quite handy for 
> my tests of incremental backups over NBD (since I frequently use 
> x-dirty-bitmap coupled with qemu-img map to read bitmaps, and 
> subsetting the output is indeed nice), so I'll queue this through my 
> NBD tree.  It may be another week or so before I send a pull request 
> including this and other collected patches.
>
> Congratulations on your first qemu contribution!
Thanks :)
>
>>  qemu-img.c                 | 76 +++++++++++++++++---------------------
>>  tests/qemu-iotests/049.out |  8 ++--
>>  2 files changed, 38 insertions(+), 46 deletions(-)
>
> This series diffstat is off; later in the series, in 4/4, I see:
I had some copy & paste issues with my cover letter, sorry.
>
>>  docs/tools/qemu-img.rst |  2 +-
>>  qemu-img-cmds.hx        |  4 ++--
>>  qemu-img.c              | 22 +++++++++++++++++++++-
>>  3 files changed, 24 insertions(+), 4 deletions(-)
>
> What I don't see is any iotest coverage of the new options, to ensure 
> they don't regress.  Either a new iotest, or an enhancement to an 
> existing iotest.  If you feel up to the task, post a 5/4 patch; if 
> not, I'll probably enhance 223 (my x-dirty-bitmap reading code 
> mentioned above).
>
I will take a look at test 223, and see if I can enhance it.

[PATCH v3 5/4] iotests: Enhance 223 to cover qemu-img map improvements

[Eric Blake]

Since qemu-img map + x-dirty-bitmap remains the easiest way to read
persistent bitmaps at the moment, it makes a reasonable place to add
coverage to ensure we do not regress on the just-added parameters to
qemu-img map.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

[Resend for proper threading, since --in=reply-to= is not the same as
--in-reply-to=]

 tests/qemu-iotests/223     | 6 ++++--
 tests/qemu-iotests/223.out | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 56fbc5fb09a0..d68bc3cb6f1a 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -2,7 +2,7 @@
 #
 # Test reading dirty bitmap over NBD
 #
-# Copyright (C) 2018-2019 Red Hat, Inc.
+# Copyright (C) 2018-2020 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -206,7 +206,9 @@ $QEMU_IMG map --output=json --image-opts \

 nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
 IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
-$QEMU_IMG map --output=json --image-opts \
+$QEMU_IMG map --output=json --image-opts --max-length=12345 \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+$QEMU_IMG map --output=json --image-opts --start-offset=12345 \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map

 # success, all done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 80c0cf65095b..e1eaaedb55b3 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -201,6 +201,7 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 [{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
-{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1024, "length": 11321, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 12345, "length": 2084807, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 *** done
-- 
2.26.2

Re: [PATCH v3 0/4] Additional parameters for qemu_img map

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200513133629.18508-1-eyal.moscovici@oracle.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/ioport.o
  CC      x86_64-softmmu/qtest.o
/tmp/qemu-test/src/qemu-img.c: In function 'cvtnum_full':
/tmp/qemu-test/src/qemu-img.c:488:63: error: format '%ld' expects argument of type 'long int', but argument 3 has type 'int64_t' {aka 'long long int'} [-Werror=format=]
         error_report("Invalid %s specified. Must be between %ld bytes "
                                                             ~~^
                                                             %lld
                      "to %ld bytes.", name, min, max);
                                             ~~~                
/tmp/qemu-test/src/qemu-img.c:488:22: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'int64_t' {aka 'long long int'} [-Werror=format=]
         error_report("Invalid %s specified. Must be between %ld bytes "
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                      "to %ld bytes.", name, min, max);
---
                          ~~^
                          %lld
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/dump/dump.o
  CC      x86_64-softmmu/memory.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=33f83b7d61224ad79355fee02312ee9c', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-z4fvcbiq/src/docker-src.2020-05-13-18.10.43.324:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=33f83b7d61224ad79355fee02312ee9c
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-z4fvcbiq/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m36.638s
user    0m7.720s


The full log is available at
http://patchew.org/logs/20200513133629.18508-1-eyal.moscovici@oracle.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 0/4] Additional parameters for qemu_img map

[Eric Blake]

On 5/13/20 5:13 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200513133629.18508-1-eyal.moscovici@oracle.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #! /bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>    CC      x86_64-softmmu/ioport.o
>    CC      x86_64-softmmu/qtest.o
> /tmp/qemu-test/src/qemu-img.c: In function 'cvtnum_full':
> /tmp/qemu-test/src/qemu-img.c:488:63: error: format '%ld' expects argument of type 'long int', but argument 3 has type 'int64_t' {aka 'long long int'} [-Werror=format=]
>           error_report("Invalid %s specified. Must be between %ld bytes "
>                                                               ~~^
>                                                               %lld
>                        "to %ld bytes.", name, min, max);

patchew is correct; printing int64_t values requires "%"PRId64 rather 
than "%ld".  I'm fine with touching that up in my queue, unless you'd 
like to submit v4.


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

[PATCH v3 5/4] iotests: Enhance 223 to cover qemu-img map improvements

[Eric Blake]

Since qemu-img map + x-dirty-bitmap remains the easiest way to read
persistent bitmaps at the moment, it makes a reasonable place to add
coverage to ensure we do not regress on the just-added parameters to
qemu-img map.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

My enthusiasm got the better of me - I whipped out this patch instead
of waiting for Eyal's response ;)

 tests/qemu-iotests/223     | 6 ++++--
 tests/qemu-iotests/223.out | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 56fbc5fb09a0..d68bc3cb6f1a 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -2,7 +2,7 @@
 #
 # Test reading dirty bitmap over NBD
 #
-# Copyright (C) 2018-2019 Red Hat, Inc.
+# Copyright (C) 2018-2020 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -206,7 +206,9 @@ $QEMU_IMG map --output=json --image-opts \

 nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG"
 IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
-$QEMU_IMG map --output=json --image-opts \
+$QEMU_IMG map --output=json --image-opts --max-length=12345 \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+$QEMU_IMG map --output=json --image-opts --start-offset=12345 \
   "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map

 # success, all done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 80c0cf65095b..e1eaaedb55b3 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -201,6 +201,7 @@ read 2097152/2097152 bytes at offset 2097152
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 [{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
-{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1024, "length": 11321, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 12345, "length": 2084807, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 *** done
-- 
2.26.2