Thanks for the review. I will send V2 based on QEMU version 5.0. On 29/04/2020, 18:06, "Eric Blake" <ebl...@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.ka...@oracle.com> > Co-developed-by: Yoav Elnekave <yoav.elnek...@oracle.com> > Signed-off-by: Yoav Elnekave <yoav.elnek...@oracle.com> > Signed-off-by: Eyal Moscovici <eyal.moscov...@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 <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org