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

[Daniel P. Berrange]

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.

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.

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.

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