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

[Kevin Wolf <kwolf@redhat.com>]

Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben:
> On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <kwolf@redhat.com> 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)?

A clean interface. There is really little overlap with what we have:

{ 'struct': 'BlockdevSnapshot',
  'data': { '*device': 'str', '*node-name': 'str',
            'snapshot-file': 'str', '*snapshot-node-name': 'str',
            '*format': 'str', '*mode': 'NewImageMode' } }

When you add an existing node which has been created with blockdev-add
as a snapshot, you won't use snapshot-file, snapshot-node-name, format
and mode. We would either have to make all of them optional and actually
forbid them when a reference is given, or to ensure that they are
consistent with the already existing node. That we have both device and
node-name (and both marked as optional, while one of them is required) is
also not in line with our current practise.

If we went further that way, the schema wouldn't really be expressive
any more because there would be too many hidden rules of which
combinations are allowed and which aren't.

What you really need for the version with a reference is just:

{ 'struct': 'BlockdevSnapshot',
  'data': { 'device': 'str', 'snapshot': 'str' } }

Where both arguments refer to a node by node-name or backend name.

Kevin