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

[Vladimir Sementsov-Ogievskiy]

07.02.2020 17:41, Max Reitz wrote:
> On 07.02.20 13:07, Vladimir Sementsov-Ogievskiy wrote:
> > 07.02.2020 13:33, Max Reitz wrote:
> > > On 04.02.20 15:23, Eric Blake wrote:
> > > > 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).
> > >
> > > Why?
> > >
> > > I could imagine a user creating a qcow2 image on some block device with
> > > preallocation where we cannot verify that the result will be zero.  But
> > > they want qemu not to zero the device, so they would specify
> > > --target-is-zero.
> >
> > If user create image, setting --target-is-zero is always valid. But if
> > we in
> > same operation create the image automatically, having --target-is-zero,
> > when
> > we know that what we are creating is not zero is misleading and should
> > fail..
>
> bdrv_has_zero_init() doesn’t return false only for images that we know
> are not zero.  It returns true for images where we know they are.  But
> if we don’t know, then it returns false also.

yes, but we don't have better check.

> > If we want to add a behavior to skip zeros unconditionally, we should
> > call new
> > option --skip-zeroes, to clearly specify what we want.
>
> It was my impression that this was exactly what --target-is-zero means
> and implies.

For me it sounds strange that user has better knowledge about what Qemu
creates than Qemu itself. And if it so - it should be fixed in Qemu,
rather than creating user interface to hint Qemu what it does.



--
Best regards,
Vladimir