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

[Eric Blake <eblake@redhat.com>]

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