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" <address@hidden> 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 <address@hidden>
    > Co-developed-by: Yoav Elnekave <address@hidden>
    > Signed-off-by: Yoav Elnekave <address@hidden>
    > Signed-off-by: Eyal Moscovici <address@hidden>
    > ---
    >   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 <address@hidden>
    
    -- 
    Eric Blake, Principal Software Engineer
    Red Hat, Inc.           +1-919-301-3226
    Virtualization:  qemu.org | libvirt.org