On Tue, Jul 18, 2017 at 03:41:21PM +0200, Markus Armbruster wrote:
> BlockdevRef is an alternate of BlockdevOptions (inline definition) and
> str (reference to an existing block device by name). BlockdevRef
> value "" is special: "no block device should be referenced." It's
> actually interpreted that way in just one place: optional member
> @backing of COW formats. Semantics:
>
> * Present means "use this block device" as backing storage
>
> * Absent means "default to the one stored in the image"
>
> * Except "" means "don't use backing storage at all"
>
> The first two are perfectly normal: when the parameter is absent, it
> defaults to an implied value, but the value's meaning is the same.
>
> The third one overloads the parameter with a second meaning. The
> overloading is *implicit*, i.e. it's not visible in the types. Works
> here, because "" is not a value block device ID.
>
> Pressing argument values the schema accepts, but are semantically
> invalid, into service to mean "do something else entirely" is not
> general, as suitable invalid values need not exist. I also find it
> ugly.
>
> To clean this up, we could add a separate flag argument to suppress
> @backing, or add a distinct value to @backing. This commit implements
> the latter: add JSON null to the values of @backing, deprecate "".
>
> Because we're so close to the 2.10 freeze, implement it in the
> stupidest way possible: have qmp_blockdev_add() rewrite null to ""
> before anything else can see the null. Works, because BlockdevRef
> occurs only within arguments of blockdev-add. The proper way to do it
> would be rewriting "" to null, preferably in a cleaner way, but that
> requires fixing up code to work with null. Add a TODO comment for
> that.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> blockdev.c | 14 ++++++++++++++
> qapi/block-core.json | 29 ++++++++++++++++++++++-------
> tests/qemu-iotests/085 | 2 +-
> tests/qemu-iotests/139 | 2 +-
> 4 files changed, 38 insertions(+), 9 deletions(-)
Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
>
> diff --git a/blockdev.c b/blockdev.c
> index 9c6dd27..1c42699 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3886,6 +3886,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error
> **errp)
> QObject *obj;
> Visitor *v = qobject_output_visitor_new(&obj);
> QDict *qdict;
> + const QDictEntry *ent;
> Error *local_err = NULL;
>
> visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> @@ -3899,6 +3900,19 @@ void qmp_blockdev_add(BlockdevOptions *options, Error
> **errp)
>
> qdict_flatten(qdict);
>
> + /*
> + * Rewrite "backing": null to "backing": ""
> + * TODO Rewrite "" to null instead, and perhaps not even here
> + */
> + for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
> + char *dot = strrchr(ent->key, '.');
> +
> + if (!strcmp(dot ? dot + 1 : ent->key, "backing")
> + && qobject_type(ent->value) == QTYPE_QNULL) {
> + qdict_put(qdict, ent->key, qstring_new());
> + }
> + }
I find it a little yucky that we're modifying the qdict, rather than
the original "options" object, but I guess this is simpler for a quick
hack
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
