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
