Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert

[Vladimir Sementsov-Ogievskiy]

04.02.2020 16:31, Eric Blake wrote:
> On 2/4/20 3:52 AM, David Edmondson wrote:
> > In many cases the target of a convert operation is a newly provisioned
> > target that the user knows is blank (filled with zeroes). In this
>
> 'filled with zeroes' is accurate for a preallocated image, but reads awkwardly 
> for a sparse image; it might be better to state 'reads as zero'.
>
> > situation there is no requirement for qemu-img to wastefully zero out
> > the entire device.
> >
> > Add a new option, --target-is-zero, allowing the user to indicate that
> > an existing target device is already zero filled.
> >
> > Signed-off-by: David Edmondson <address@hidden>
> > ---
> >   docs/interop/qemu-img.rst |  8 +++++++-
> >   qemu-img-cmds.hx          |  4 ++--
> >   qemu-img.c                | 25 ++++++++++++++++++++++---
> >   3 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
> > index fa27e5c7b453..99bdbe4740ee 100644
> > --- a/docs/interop/qemu-img.rst
> > +++ b/docs/interop/qemu-img.rst
> > @@ -214,6 +214,12 @@ Parameters to convert subcommand:
> >     will still be printed.  Areas that cannot be read from the source will be
> >     treated as containing only zeroes.
> > +.. option:: --target-is-zero
> > +
> > +  Assume that the destination is filled with zeros. This parameter is
>
> Spelled 'zeroes' just three lines earlier.
>
> > +  mutually exclusive with the use of a backing file. It is required to
> > +  also use the ``-n`` parameter to skip image creation.
>
> I understand that it is safer to have restrictions now and lift them later, 
> than to allow use of the option at any time and leave room for the user to 
> shoot themselves in the foot with no way to add safety later.  The argument 
> against no backing file is somewhat understandable (technically, as long as the 
> backing file also reads as all zeroes, then the overall image reads as all 
> zeroes - but why have a backing file that has no content?); the argument 
> requiring -n is a bit weaker (if I'm creating an image, I _know_ it reads as 
> all zeroes, so the --target-is-zero argument is redundant, but it shouldn't 
> hurt to allow it).

I know that it reads as all zeroes, only if this format provides zero 
initialization..

> > +++ b/qemu-img.c
>
> > @@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
> >           warn_report("This will become an error in future QEMU versions.");
> >       }
> > +    if (s.has_zero_init && !skip_create) {
> > +        error_report("--target-is-zero requires use of -n flag");
> > +        goto fail_getopt;
> > +    }
>
> So I think we could drop this hunk with no change in behavior.

I think, no we can't. If we allow target-is-zero, with -n, we'd better to check 
that what we are creating is zero-initialized (format has zero-init), and if 
not we should report error.

> > +
> >       s.src_num = argc - optind - 1;
> >       out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
> > @@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
> >       }
> >       s.target_has_backing = (bool) out_baseimg;
> > +    if (s.has_zero_init && s.target_has_backing) {
> > +        error_report("Cannot use --target-is-zero with a backing file");
> > +        goto out;
> > +    }
> > +
>
> while keeping this one makes sense.  At any rate, typo fixes are minor, and 
> whether or not we drop the hunk I claim is a needless restriction against 
> redundancy,
>
> Reviewed-by: Eric Blake <address@hidden>


--
Best regards,
Vladimir