Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 03:07:15PM +0100, Kevin Wolf wrote:
> Am 15.01.2018 um 14:51 hat Daniel P. Berrange geschrieben:
> > On Mon, Jan 15, 2018 at 02:38:48PM +0100, Kevin Wolf wrote:
> > > Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
> > > > On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> > > > > Signed-off-by: Kevin Wolf <address@hidden>
> > > > > ---
> > > > >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > index 1749376c61..9341f6708d 100644
> > > > > --- a/qapi/block-core.json
> > > > > +++ b/qapi/block-core.json
> > > > > @@ -3320,6 +3320,37 @@
> > > > >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> > > > >  
> > > > >  ##
> > > > > +# @BlockdevQcow2CompatLevel:
> > > > > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 
> > > > > (version 2)
> > > > > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 
> > > > > (version 3)
> > > > > +#
> > > > > +# Since: 2.10
> > > > > +##
> > > > > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > > > > +  'data': [ '0_10', '1_1' ] }
> > > > > +
> > > > > +
> > > > > +##
> > > > > +# @BlockdevCreateOptionsQcow2:
> > > > > +#
> > > > > +# Driver specific image creation options for qcow2.
> > > > > +#
> > > > > +# TODO Describe fields
> > > > > +#
> > > > > +# Since: 2.12
> > > > > +##
> > > > > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > > > > +  'data': { 'size':             'size',
> > > > > +            '*compat':          'BlockdevQcow2CompatLevel',
> > > > > +            '*backing-file':    'str',
> > > > > +            '*backing-fmt':     'BlockdevDriver',
> > > > 
> > > > For anything non-trivial, the caller is going to have to stuff a
> > > > JSON string into 'backing-file' value. It feels like we should
> > > > be referencing 'BlockdevOptions' here in some manner.
> > > 
> > > Hm, that's an interesting question. For the image creation, this is
> > > really treated as a string that is directly written into the image file,
> > > without being parsed, so 'str' is the more correct type in this context.
> > > However, when the backing file gets loaded, that string is in fact
> > > parsed and we expect it to describe the same thing as BlockdevOptions.
> > > 
> > > If we get BlockdevOptions here, qemu would have to convert them into a
> > > json:{...} string before writing the header of the new image.
> > > Compatibility code would become a bit more complex because we'd have to
> > > convert the existing string into BlockdevOptions, only to convert it
> > > back to a string before we write it to the image file. And finally, the
> > > 1023 character limit of qcow2 becomes kind of unpredicatble when you
> > > don't pass the string yourself.
> > > 
> > > So considering all of that, I still think that 'str' is the better
> > > option here.
> > 
> > Hmm, when we write the backing chain into the qcow2 header, we only
> > want to write the 1st level of the backing chain.
> 
> That's a good point, too. References in BlockdevOptions are often
> mandatory, which conflicts with this.
> 
> > When we are creating the new qcow2 image, we could be pointing to a backing
> > chain that goes many levels deep. So the actual creation process potentially
> > needs to be given the full arbitrarily deep backing file eg in order that
> > we can set 'encrypt.secret' for any encrypted images at at arbitrary level.
> 
> But we don't even access the images in the backing chain during image
> creation. Why would we need a secret for them?

Oh, i forgot that when qcow2 opens the just created image, it uses the
O_NO_IO and O_NO_BACKING flags. So yeah, we're probably ok in actual
fact.

> > IOW, I think we need to be able to pass BlockdevOptions here to specify the
> > full deep chain, but then only the 1st level of these BlockdevOptions should
> > get written into the qcow2 file header.
> 
> But what's the point of even passing the full chain if only the first
> layer is actually used?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|