Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2019-10-07 at 10:04 +0200, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
> > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > Currently only for changing crypto parameters
> > 
> > Yep, that elegantly avoids most of the problems we’d have otherwise. :-)
> > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> [...]
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 4a6db98938..0eb4e45168 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -4294,6 +4294,7 @@
> > >  # Driver specific image creation options for qcow2.
> > >  #
> > >  # @file             Node to create the image format on
> > > +#                   Mandatory for create
> > >  # @data-file        Node to use as an external data file in which all guest
> > >  #                   data is stored so that only metadata remains in the qcow2
> > >  #                   file (since: 4.0)
> > > @@ -4301,6 +4302,7 @@
> > >  #                   standalone (read-only) raw image without looking at qcow2
> > >  #                   metadata (default: false; since: 4.0)
> > >  # @size             Size of the virtual disk in bytes
> > > +#                   Mandatory for create
> > >  # @version          Compatibility level (default: v3)
> > >  # @backing-file     File name of the backing file if a backing file
> > >  #                   should be used
> > > @@ -4315,10 +4317,10 @@
> > >  # Since: 2.12
> > >  ##
> > >  { 'struct': 'BlockdevCreateOptionsQcow2',
> > > -  'data': { 'file':             'BlockdevRef',
> > > +  'data': { '*file':            'BlockdevRef',
> > >              '*data-file':       'BlockdevRef',
> > >              '*data-file-raw':   'bool',
> > > -            'size':             'size',
> > > +            '*size':            'size',
> > >              '*version':         'BlockdevQcow2Version',
> > >              '*backing-file':    'str',
> > >              '*backing-fmt':     'BlockdevDriver',
> > > 
> 
> My comments to the previous patch apply.
> 
> > Making these fields optional makes me wonder whether it actually make
> > sense to have both create and amend share a single QAPI structure.
> > Wouldn’t it better to have a separate amend structure that then actually
> > reflects what we support?  (This would also solve the problem of how to
> > express in the code what is and what isn’t supported through
> > blockdev-amend.)
> 
> Good point.  As is, the schema is rather confusing, at least to me.  We
> reduce or avoid the confusion in documentation or in code.  I'd prefer
> code unless it leads to too much duplication.  "Too much" is of course
> subjective.  Maxim, would you mind exploring it for us?

Nothing against having a separate amend structure, I actually prefer this,
and I don't think it will complicate the code. 


Best regards,
	Maxim Levitsky