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

[Alberto Garcia]

On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <address@hidden> 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).
>
> Yes, this is what we should do.

Sounds like a good idea, thanks for the feedback !

>> >> +# @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.

This is not necessary if we go for the "reference" mode proposed by
Markus, since the "options" parameter would disappear.

> Let's avoid such magic and instead add a new, clean blockdev-* style
> command. Maybe call it simply blockdev-snapshot; the -sync part was
> added because we knew it wouldn't be the final version of the command.
> Now we don't have any bdrv_open() in it any more that could by
> synchronous or asynchronous.

Would you then prefer me to create a new command instead of extending
the existing one? What would be the benefit (other than a better name)?

Berto