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

[Eric Blake]

On 2/4/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:

> > 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.

Good call.  Yes, if we allow --target-is-zero without -n, we MUST insist 
that bdrv_has_zero_init() returns 1 (or, after my followup series, 
bdrv_known_zeroes() includes BDRV_ZERO_CREATE).  I can work that into v2 
of my series, if desired.

> > > +
> > >       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>

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