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

[Markus Armbruster <armbru@redhat.com>]

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?