[PATCH 0/2] Additional parameters for qemu_img map

[PATCH 0/2] 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.

Eyal Moscovici (2):
  qemu-img: refactor dump_map_entry JSON format output
  qemu-img: Add --start-offset and --max-length to map

 docs/tools/qemu-img.rst |  2 +-
 qemu-img-cmds.hx        |  4 ++--
 qemu-img.c              | 42 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH 1/2] 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.

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 afddf33f08..9cf8576217 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2860,9 +2860,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");
@@ -2871,8 +2870,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         putchar('}');
 
-        if (!next) {
-            printf("]\n");
+        if (next) {
+            printf(",\n");
         }
         break;
     }
@@ -3047,6 +3046,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) {
+        printf("[");
     }
 
     length = blk_getlength(blk);
@@ -3078,6 +3079,9 @@ static int img_map(int argc, char **argv)
     }
 
     ret = dump_map_entry(output_format, &curr, NULL);
+    if (output_format == OFORMAT_JSON) {
+        printf("]\n");
+    }
 
 out:
     blk_unref(blk);
-- 
2.17.2 (Apple Git-113)

[PATCH 2/2] 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.

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              | 30 +++++++++++++++++++++++++++++-
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76..924e89f679 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 9cf8576217..cd365b275e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2967,6 +2967,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;
@@ -2979,9 +2981,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;
@@ -3005,6 +3009,26 @@ static int img_map(int argc, char **argv)
         case OPTION_OUTPUT:
             output = optarg;
             break;
+        case 's':
+            start_offset = cvtnum(optarg);
+            if (start_offset < 0) {
+                error_report("Invalid start offset specified! You may use "
+                             "k, M, G, T, P or E suffixes for ");
+                error_report("kilobytes, megabytes, gigabytes, terabytes, "
+                             "petabytes and exabytes.");
+                return 1;
+            }
+            break;
+        case 'l':
+            max_length = cvtnum(optarg);
+            if (max_length < 0) {
+                error_report("Invalid max length specified! You may use "
+                             "k, M, G, T, P or E suffixes for ");
+                error_report("kilobytes, megabytes, gigabytes, terabytes, "
+                             "petabytes and exabytes.");
+                return 1;
+            }
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3050,7 +3074,11 @@ static int img_map(int argc, char **argv)
         printf("[");
     }
 
+    curr.start = start_offset;
     length = blk_getlength(blk);
+    if (max_length != -1) {
+        length = MIN(start_offset + max_length, length);
+    }
     while (curr.start + curr.length < length) {
         int64_t offset = curr.start + curr.length;
         int64_t n;
-- 
2.17.2 (Apple Git-113)

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

[John Snow]

Hi, this series is 30 days old and didn't attract any replies. As the
5.0 release has just shipped, it may be re-appropriate to resubmit this
series for inclusion in the next release.

On 3/22/20 5:11 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.
> 
> Eyal Moscovici (2):
>   qemu-img: refactor dump_map_entry JSON format output
>   qemu-img: Add --start-offset and --max-length to map
> 
>  docs/tools/qemu-img.rst |  2 +-
>  qemu-img-cmds.hx        |  4 ++--
>  qemu-img.c              | 42 ++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 40 insertions(+), 8 deletions(-)
> 

-- 
—js

Re: [PATCH 1/2] qemu-img: refactor dump_map_entry JSON format output

[Eric Blake]

On 3/22/20 4:11 AM, Eyal Moscovici wrote:
> 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.
> 
> 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(-)
> 

> @@ -2871,8 +2870,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
>           }
>           putchar('}');
>   
> -        if (!next) {
> -            printf("]\n");
> +        if (next) {
> +            printf(",\n");

As long as you're touching this, puts(",") is slightly more efficient 
than printf().  But what you have is not wrong.

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 2/2] qemu-img: Add --start-offset and --max-length to map

[Eric Blake]

On 3/22/20 4:11 AM, Eyal Moscovici wrote:
> 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.
> 
> 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              | 30 +++++++++++++++++++++++++++++-
>   3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 0080f83a76..924e89f679 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

Consistency with the rest of the line says this should be 
[--start-offset=OFFSET] [--max-length=LEN]

>   
>     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")

this one is fine,

>   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

this one has the same problem as the .rst.

> @@ -3005,6 +3009,26 @@ static int img_map(int argc, char **argv)
>           case OPTION_OUTPUT:
>               output = optarg;
>               break;
> +        case 's':
> +            start_offset = cvtnum(optarg);
> +            if (start_offset < 0) {
> +                error_report("Invalid start offset specified! You may use "
> +                             "k, M, G, T, P or E suffixes for ");
> +                error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                             "petabytes and exabytes.");

Pre-existing elsewhere in the file, but this seems rather verbose - 
shouldn't we have cvtnum() (or another wrapper function) give this extra 
information about what is valid, rather than open-coding it at every 
client of cvtnum()?

> +                return 1;
> +            }
> +            break;
> +        case 'l':
> +            max_length = cvtnum(optarg);
> +            if (max_length < 0) {
> +                error_report("Invalid max length specified! You may use "
> +                             "k, M, G, T, P or E suffixes for ");
> +                error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                             "petabytes and exabytes.");
> +                return 1;
> +            }
> +            break;
>           case OPTION_OBJECT: {
>               QemuOpts *opts;
>               opts = qemu_opts_parse_noisily(&qemu_object_opts,
> @@ -3050,7 +3074,11 @@ static int img_map(int argc, char **argv)
>           printf("[");
>       }
>   
> +    curr.start = start_offset;
>       length = blk_getlength(blk);
> +    if (max_length != -1) {
> +        length = MIN(start_offset + max_length, length);
> +    }

Pre-existing, but where does this code check for length == -1?  But your 
MIN() doesn't make it any worse (if we fail to get length, we merely 
skip the loop).

>       while (curr.start + curr.length < length) {
>           int64_t offset = curr.start + curr.length;
>           int64_t n;
> 

Overall, the idea makes sense to me.  But I'm not sure which maintainer 
should actually incorporate the patch.

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 2/2] qemu-img: Add --start-offset and --max-length to map

[Eyal Moscovici]

Thanks for the review. I will send V2 based on QEMU version 5.0.

On 29/04/2020, 18:06, "Eric Blake" <eblake@redhat.com> wrote:

    On 3/22/20 4:11 AM, Eyal Moscovici wrote:
    > 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.
    > 
    > 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              | 30 +++++++++++++++++++++++++++++-
    >   3 files changed, 32 insertions(+), 4 deletions(-)
    > 
    > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
    > index 0080f83a76..924e89f679 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
    
    Consistency with the rest of the line says this should be 
    [--start-offset=OFFSET] [--max-length=LEN]

Will fix.
    
    >   
    >     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")
    
    this one is fine,
    
    >   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
    
    this one has the same problem as the .rst.
    
    > @@ -3005,6 +3009,26 @@ static int img_map(int argc, char **argv)
    >           case OPTION_OUTPUT:
    >               output = optarg;
    >               break;
    > +        case 's':
    > +            start_offset = cvtnum(optarg);
    > +            if (start_offset < 0) {
    > +                error_report("Invalid start offset specified! You may use "
    > +                             "k, M, G, T, P or E suffixes for ");
    > +                error_report("kilobytes, megabytes, gigabytes, terabytes, "
    > +                             "petabytes and exabytes.");
    
    Pre-existing elsewhere in the file, but this seems rather verbose - 
    shouldn't we have cvtnum() (or another wrapper function) give this extra 
    information about what is valid, rather than open-coding it at every 
    client of cvtnum()?

You are probably right I will send a patch that adds the error message  about the units to cvtnum().
    
    > +                return 1;
    > +            }
    > +            break;
    > +        case 'l':
    > +            max_length = cvtnum(optarg);
    > +            if (max_length < 0) {
    > +                error_report("Invalid max length specified! You may use "
    > +                             "k, M, G, T, P or E suffixes for ");
    > +                error_report("kilobytes, megabytes, gigabytes, terabytes, "
    > +                             "petabytes and exabytes.");
    > +                return 1;
    > +            }
    > +            break;
    >           case OPTION_OBJECT: {
    >               QemuOpts *opts;
    >               opts = qemu_opts_parse_noisily(&qemu_object_opts,
    > @@ -3050,7 +3074,11 @@ static int img_map(int argc, char **argv)
    >           printf("[");
    >       }
    >   
    > +    curr.start = start_offset;
    >       length = blk_getlength(blk);
    > +    if (max_length != -1) {
    > +        length = MIN(start_offset + max_length, length);
    > +    }
    
    Pre-existing, but where does this code check for length == -1?  But your 
    MIN() doesn't make it any worse (if we fail to get length, we merely 
    skip the loop).

I don't think there is a check. I will add a check in a different patch.
    
    >       while (curr.start + curr.length < length) {
    >           int64_t offset = curr.start + curr.length;
    >           int64_t n;
    > 
    
    Overall, the idea makes sense to me.  But I'm not sure which maintainer 
    should actually incorporate the patch.
    
    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 1/2] qemu-img: refactor dump_map_entry JSON format output

[Eyal Moscovici]

On 29/04/2020, 17:58, "Eric Blake" <eblake@redhat.com> wrote:

    On 3/22/20 4:11 AM, Eyal Moscovici wrote:
    > 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.
    > 
    > 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(-)
    > 
    
    > @@ -2871,8 +2870,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
    >           }
    >           putchar('}');
    >   
    > -        if (!next) {
    > -            printf("]\n");
    > +        if (next) {
    > +            printf(",\n");
    
    As long as you're touching this, puts(",") is slightly more efficient 
    than printf().  But what you have is not wrong.

Thanks, will fix.
    
    Reviewed-by: Eric Blake <eblake@redhat.com>
    
    -- 
    Eric Blake, Principal Software Engineer
    Red Hat, Inc.           +1-919-301-3226
    Virtualization:  qemu.org | libvirt.org
    
    

[PATCH v2 0/5] 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.

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

Eyal Moscovici (5):
  qemu-img: remove check that cvtnum value > MAX_INT
  qemu_img: add error report to cvtnum
  qemu-img: validate image length in img_map
  qemu-img: refactor dump_map_entry JSON format output
  qemu-img: Add --start-offset and --max-length to map

 docs/tools/qemu-img.rst    |   2 +-
 qemu-img-cmds.hx           |   4 +-
 qemu-img.c                 | 106 ++++++++++++++++++++++---------------
 tests/qemu-iotests/049.out |   4 +-
 4 files changed, 67 insertions(+), 49 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eyal Moscovici]

Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 (util/cutils: Change
qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
cvtnum. As a result there is no need to check it separately outside of cvtnum.

Acked-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
---
 qemu-img.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..116a9c6349 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
+            if (sval < 0) {
                 error_report("Invalid buffer size specified");
                 return 1;
             }
@@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0 || sval > INT_MAX) {
+            if (sval < 0) {
                 error_report("Invalid step size specified");
                 return 1;
             }
@@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
 
     res = cvtnum(arg);
 
-    if (res <= 0 || res > INT_MAX) {
+    if (res <= 0) {
         error_report("invalid number: '%s'", arg);
         return 1;
     }
-- 
2.17.2 (Apple Git-113)

[PATCH v2 2/5] qemu_img: add error report to cvtnum

[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 to reduce duplicate code and
provide a single error message.

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

diff --git a/qemu-img.c b/qemu-img.c
index 116a9c6349..4a06ab7fe8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -470,16 +470,22 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
     return 0;
 }
 
-static int64_t cvtnum(const char *s)
+static int64_t cvtnum(const char *arg_name, const char *arg_value)
 {
     int err;
     uint64_t value;
 
-    err = qemu_strtosz(s, NULL, &value);
-    if (err < 0) {
+    err = qemu_strtosz(arg_value, NULL, &value);
+    if (err < 0 && err != -ERANGE) {
+        error_report("Invalid %s specified! You may use "
+                     "k, M, G, T, P or E suffixes for ", arg_name);
+        error_report("kilobytes, megabytes, gigabytes, terabytes, "
+                     "petabytes and exabytes.");
         return err;
     }
-    if (value > INT64_MAX) {
+    if (err == -ERANGE || value > INT64_MAX) {
+        error_report("Invalid %s specified! Must be less than 8 EiB!",
+                     arg_name);
         return -ERANGE;
     }
     return value;
@@ -572,16 +578,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 +2185,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 +4291,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 +4305,8 @@ static int img_bench(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
+            sval = cvtnum("buffer size", optarg);
             if (sval < 0) {
-                error_report("Invalid buffer size specified");
                 return 1;
             }
 
@@ -4319,9 +4317,8 @@ static int img_bench(int argc, char **argv)
         {
             int64_t sval;
 
-            sval = cvtnum(optarg);
+            sval = cvtnum("step_size", optarg);
             if (sval < 0) {
-                error_report("Invalid step size specified");
                 return 1;
             }
 
@@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
 {
     int64_t res;
 
-    res = cvtnum(arg);
+    res = cvtnum("bs", arg);
 
-    if (res <= 0) {
-        error_report("invalid number: '%s'", arg);
+    if (res < 0) {
+        return 1;
+    } else if (res == 0) {
+        error_report("Invalid bs specified! It cannot be 0.");
         return 1;
     }
     in->bsz = out->bsz = res;
@@ -4506,10 +4505,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 +4536,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 +4920,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..0ec2e6b82e 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -92,13 +92,13 @@ 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 less than 8 EiB!
 
 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 less than 8 EiB!
 
 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'
-- 
2.17.2 (Apple Git-113)

[PATCH v2 3/5] 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.

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 4a06ab7fe8..a1b507a0be 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3086,6 +3086,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)

[PATCH v2 4/5] 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 a1b507a0be..0a140fe564 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2896,9 +2896,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");
@@ -2907,8 +2906,8 @@ static int dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         putchar('}');
 
-        if (!next) {
-            printf("]\n");
+        if (next) {
+            puts(",");
         }
         break;
     }
@@ -3083,6 +3082,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);
@@ -3119,6 +3120,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 v2 5/5] 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 0a140fe564..f59b2c0a7c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3003,6 +3003,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;
@@ -3015,9 +3017,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;
@@ -3041,6 +3045,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,
@@ -3091,7 +3107,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 v2 0/5] Additional parameters for qemu_img map

[Eric Blake]

On 5/6/20 4:34 PM, 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.
> 
> V2 changes:
> 1. add error reporting to cvtnum.
> 2. add image length validation in img_map.
> 3. rebase over QEMU 5.0.

It's better to post a v2 as a new top-level thread rather than buried 
in-reply-to the v1 thread; among other things, burying a reply can cause 
automated patch tooling to miss the updated series.

But since I see it, I'll review.

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

Re: [PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eric Blake]

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 (util/cutils: Change
> qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
> cvtnum. As a result there is no need to check it separately outside of cvtnum.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>   qemu-img.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6a4327aaba..116a9c6349 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>               int64_t sval;
>   
>               sval = cvtnum(optarg);
> -            if (sval < 0 || sval > INT_MAX) {
> +            if (sval < 0) {
>                   error_report("Invalid buffer size specified");

INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code change 
allows larger buffer sizes, which is probably not a good idea.

>                   return 1;
>               }
> @@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
>               int64_t sval;
>   
>               sval = cvtnum(optarg);
> -            if (sval < 0 || sval > INT_MAX) {
> +            if (sval < 0) {
>                   error_report("Invalid step size specified");
>                   return 1;
>               }
> @@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
>   
>       res = cvtnum(arg);
>   
> -    if (res <= 0 || res > INT_MAX) {
> +    if (res <= 0) {
>           error_report("invalid number: '%s'", arg);
>           return 1;
>       }
> 

NACK.

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

Re: [PATCH v2 2/5] qemu_img: add error report to cvtnum

[Eric Blake]

On 5/6/20 4:34 PM, 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 to reduce duplicate code and
> provide a single error message.
> 
> Acked-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
> ---
>   qemu-img.c                 | 63 ++++++++++++++++----------------------
>   tests/qemu-iotests/049.out |  4 +--
>   2 files changed, 28 insertions(+), 39 deletions(-)
> 

> 
> -    err = qemu_strtosz(s, NULL, &value);
> -    if (err < 0) {
> +    err = qemu_strtosz(arg_value, NULL, &value);
> +    if (err < 0 && err != -ERANGE) {
> +        error_report("Invalid %s specified! You may use "
> +                     "k, M, G, T, P or E suffixes for ", arg_name);
> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
> +                     "petabytes and exabytes.");
>          return err;
>      }
> -    if (value > INT64_MAX) {
> +    if (err == -ERANGE || value > INT64_MAX) {
> +        error_report("Invalid %s specified! Must be less than 8 EiB!",

Copied from our pre-existing errors, but why are we shouting at our 
user?  This would be a good time to s/!/./ to tone it down a bit.

> @@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
>   {
>       int64_t res;
>   
> -    res = cvtnum(arg);
> +    res = cvtnum("bs", arg);
>   
> -    if (res <= 0) {
> -        error_report("invalid number: '%s'", arg);
> +    if (res < 0) {
> +        return 1;
> +    } else if (res == 0) {
> +        error_report("Invalid bs specified! It cannot be 0.");

Maybe it's worth two functions:

int64_t cvtnum_full(const char *name, const char *value, int64_t min, 
int64_t max)

and then a common helper:

int64_t cvtnum(const char *name, const char *value) {
     return cvtnum_full(name, value, 0, INT64_MAX);
}

many existing callers remain with cvtnum(), but callers like this could 
be cvtnum("bs", arg, 1, INT64_MAX).  You'd still have to special-case 
other restrictions, such as whether a number must a power-of-2, but 
that's fewer places.

> +++ b/tests/qemu-iotests/049.out
> @@ -92,13 +92,13 @@ 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 less than 8 EiB!

Nice that you checked for iotest fallout.  Is this really the only 
impacted test?

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

Re: [PATCH v2 3/5] qemu-img: validate image length in img_map

[Eric Blake]

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> The code handles this case correctly we merely skip the loop. However it
> is probably best to return an explicit error.
> 
> 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(+)

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

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4a06ab7fe8..a1b507a0be 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3086,6 +3086,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

Re: [PATCH v2 5/5] qemu-img: Add --start-offset and --max-length to map

[Eric Blake]

On 5/6/20 4:34 PM, Eyal Moscovici wrote:
> 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>

This patch has some changes from v1.  Among others,...

> @@ -3041,6 +3045,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;

the new semantics of cvtnum() in this series is enough of a difference 
that I would have removed R-b to make sure the updated patch gets 
re-reviewed, if it had been me as author.  But in this case, it does 
look like the changes are all addressed to comments I suggested in v1, 
so I'm fine that you left my R-b.

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

Re: [PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eyal Moscovici]

On 07/05/2020 0:49, Eric Blake wrote:
> On 5/6/20 4:34 PM, Eyal Moscovici wrote:
>> Following commit f46bfdbfc8f95cf65d7818ef68a801e063c40332 
>> (util/cutils: Change
>> qemu_strtosz*() from int64_t to uint64_t) which added a similar check to
>> cvtnum. As a result there is no need to check it separately outside 
>> of cvtnum.
>>
>> Acked-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>>   qemu-img.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 6a4327aaba..116a9c6349 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid buffer size specified");
>
> INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
> change allows larger buffer sizes, which is probably not a good idea.
I was the most hesitant about this patch because of the size difference. 
I decided to submit it because the type is int64 which pairs better with 
the MAX_INT64 check and I couldn't find a concrete reason to cap the 
variable at MAX_INT. Do you a concrete reason? Because the max size 
should rerally come into effect on very fringe cases and if you are 
asking for a really big buffer you should know the risks.
>
>>                   return 1;
>>               }
>> @@ -4320,7 +4320,7 @@ static int img_bench(int argc, char **argv)
>>               int64_t sval;
>>                 sval = cvtnum(optarg);
>> -            if (sval < 0 || sval > INT_MAX) {
>> +            if (sval < 0) {
>>                   error_report("Invalid step size specified");
>>                   return 1;
>>               }
>> @@ -4493,7 +4493,7 @@ static int img_dd_bs(const char *arg,
>>         res = cvtnum(arg);
>>   -    if (res <= 0 || res > INT_MAX) {
>> +    if (res <= 0) {
>>           error_report("invalid number: '%s'", arg);
>>           return 1;
>>       }
>>
>
> NACK.
>

Re: [PATCH v2 2/5] qemu_img: add error report to cvtnum

[Eyal Moscovici]

On 07/05/2020 0:59, Eric Blake wrote:
> On 5/6/20 4:34 PM, 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 to reduce duplicate 
>> code and
>> provide a single error message.
>>
>> Acked-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Eyal Moscovici <eyal.moscovici@oracle.com>
>> ---
>>   qemu-img.c                 | 63 ++++++++++++++++----------------------
>>   tests/qemu-iotests/049.out |  4 +--
>>   2 files changed, 28 insertions(+), 39 deletions(-)
>>
>
>>
>> -    err = qemu_strtosz(s, NULL, &value);
>> -    if (err < 0) {
>> +    err = qemu_strtosz(arg_value, NULL, &value);
>> +    if (err < 0 && err != -ERANGE) {
>> +        error_report("Invalid %s specified! You may use "
>> +                     "k, M, G, T, P or E suffixes for ", arg_name);
>> +        error_report("kilobytes, megabytes, gigabytes, terabytes, "
>> +                     "petabytes and exabytes.");
>>          return err;
>>      }
>> -    if (value > INT64_MAX) {
>> +    if (err == -ERANGE || value > INT64_MAX) {
>> +        error_report("Invalid %s specified! Must be less than 8 EiB!",
>
> Copied from our pre-existing errors, but why are we shouting at our 
> user?  This would be a good time to s/!/./ to tone it down a bit.
Sure, will fix.
>
>> @@ -4491,10 +4488,12 @@ static int img_dd_bs(const char *arg,
>>   {
>>       int64_t res;
>>   -    res = cvtnum(arg);
>> +    res = cvtnum("bs", arg);
>>   -    if (res <= 0) {
>> -        error_report("invalid number: '%s'", arg);
>> +    if (res < 0) {
>> +        return 1;
>> +    } else if (res == 0) {
>> +        error_report("Invalid bs specified! It cannot be 0.");
>
> Maybe it's worth two functions:
>
> int64_t cvtnum_full(const char *name, const char *value, int64_t min, 
> int64_t max)
>
> and then a common helper:
>
> int64_t cvtnum(const char *name, const char *value) {
>     return cvtnum_full(name, value, 0, INT64_MAX);
> }
>
> many existing callers remain with cvtnum(), but callers like this 
> could be cvtnum("bs", arg, 1, INT64_MAX).  You'd still have to 
> special-case other restrictions, such as whether a number must a 
> power-of-2, but that's fewer places.
>
Great idea, I will create two functions.
>> +++ b/tests/qemu-iotests/049.out
>> @@ -92,13 +92,13 @@ 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 less than 8 EiB!
>
> Nice that you checked for iotest fallout.  Is this really the only 
> impacted test?
>
Thanks, yes.

Re: [PATCH v2 5/5] qemu-img: Add --start-offset and --max-length to map

[Eyal Moscovici]

On 07/05/2020 1:04, Eric Blake wrote:
> On 5/6/20 4:34 PM, Eyal Moscovici wrote:
>> 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>
>
> This patch has some changes from v1.  Among others,...
>
>> @@ -3041,6 +3045,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;
>
> the new semantics of cvtnum() in this series is enough of a difference 
> that I would have removed R-b to make sure the updated patch gets 
> re-reviewed, if it had been me as author.  But in this case, it does 
> look like the changes are all addressed to comments I suggested in v1, 
> so I'm fine that you left my R-b.
>
Ok, got it.

Re: [PATCH v2 1/5] qemu-img: remove check that cvtnum value > MAX_INT

[Eric Blake]

On 5/12/20 4:39 AM, Eyal Moscovici wrote:

>>> +++ b/qemu-img.c
>>> @@ -4307,7 +4307,7 @@ static int img_bench(int argc, char **argv)
>>>               int64_t sval;
>>>                 sval = cvtnum(optarg);
>>> -            if (sval < 0 || sval > INT_MAX) {
>>> +            if (sval < 0) {
>>>                   error_report("Invalid buffer size specified");
>>
>> INT_MAX is smaller than cvtnum's check for INT64_MAX.  This code 
>> change allows larger buffer sizes, which is probably not a good idea.
> I was the most hesitant about this patch because of the size difference. 
> I decided to submit it because the type is int64 which pairs better with 
> the MAX_INT64 check and I couldn't find a concrete reason to cap the 
> variable at MAX_INT. Do you a concrete reason? Because the max size 
> should rerally come into effect on very fringe cases and if you are 
> asking for a really big buffer you should know the risks.

The commit message does not call out a change in behavior.  If you are 
sure that you want a change in behavior, then justify that: mention that 
your patch specifically changes the behavior to allow larger buffers, 
and why that is okay.  Otherwise, it looks like your patch is accidental 
in its behavior change, which costs reviewer time.

In this particular case, I see no reason to allow larger buffers.  That 
is, the existing limits made sense (benchmarking anything larger than 
the maximum qcow2 cluster size of 2M is unlikely to produce useful 
results, let alone something as large as 2G, so allowing > 2G is not 
helping the user).  So even if your commit message did a good job of 
explaining that the change was intentional, it is a tough sell why we 
need it.  If all your commit is intended to do is refactoring, then it 
should not be changing behavior.


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