Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Eric Blake]

On 08/31/2015 01:53 PM, Max Reitz wrote:

> Design question: Would it make sense to instead add a "reference" mode
> to blockdev-snapshot-sync where you can specify a BDS's node-name
> instead of snapshot-file to use an existing BDS as the new top layer,
> ideally an empty one?

Indeed - then blockdev-add can be used to create an unattached BDS (with
all appropriate options), and blockdev-snapshot-sync would then attach
that BDS as the snapshot-file that wraps an existing BDS (without
needing to worry about options).

> 
> What we'd then need is a QMP command for creating images. But as far as
> I know we'll need that anyway sooner or later...

Can't blockdev-add already be used for that (at least, for supported
file types)? If not, what would it take to get it there?

> 
> 
> Comments on the patch itself below.
> 

> 
> With has_format == false, format will be set to "qcow2" by default. So,
> if the user does not specify the format explicitly, the "driver" field
> has to be set to "qcow2".
> 
> I'd rather make specifying @format and @options exclusive, and if
> @options has been specified, its "driver" field should override the
> "format" default.
> 

>> +++ b/qapi/block-core.json
>> @@ -697,11 +697,18 @@
>>  #
>>  # @mode: #optional whether and how QEMU should create a new image, default 
>> is
>>  #        'absolute-paths'.
>> +#
>> +# @options: #optional options for the new device, with the following
>> +#           restrictions for the fields: 'driver' must match the value
>> +#           of @format,
> 
> As said above, I'd rather make specifying both @options and @format
> exclusive.
> 
> Maybe there is even some QAPI magic to enforce that (and for
> 'node-name', too), I don't know...

Not that I know of at the moment, but not to say we can't add some.  The
closest we can get is with a flat union, but that requires a
non-optional discriminator field.  Maybe we can tweak qapi to make the
discriminator optional (with a default value).  Thankfully, it sounds
like Markus' work on introspection would at least let management apps
learn about a new 'options' argument.

>>                          'node-name' and @snapshot-node-name cannot be
>> +#           specified at the same time, and 'file' can only contain an
>> +#           empty string (Since 2.5)

I really don't like the idea of requiring the user to pass in an empty
string. Can we make the field optional instead, and require it to be
omitted in the context where it would otherwise need to be empty, while
still requiring it to be present in existing clients that weren't
prepared for it to be optional?  It also feels a bit ad hoc that you
describe that driver/format must be identical, but that other fields
must be mutually exclusive or the empty string; consistency would argue
that since you already handle duplication of driver/format by requiring
them to be the same, that you should also allow duplication and
validation of identical other fields that overlap.  (But even better
would be finding a way to not allow overlapping fields to appear in the
first place).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature