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

[Kevin Wolf]

Am 31.08.2015 um 22:05 hat Eric Blake geschrieben:
> 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).

Yes, this is what we should do.

The existing blockdev-snapshot-sync should really have been called
something like drive-snapshot, it doesn't belong in the blockdev-*
family of commands which works only with existing nodes.

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

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.

Kevin

pgpz_AggH_1yG.pgp
Description: PGP signature