migrate-set-parameters sets migration parameters according to is
arguments like this:
* Present means "set the parameter to this value"
* Absent means "leave the parameter unchanged"
* Except for parameters tls_creds and tls_hostname, "" means "reset
the parameter to its default value
The first two are perfectly normal: presence of the parameter makes
the command do something.
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 neither a valid TLS credentials ID, nor a valid
host name.
Pressing argument values the schema accepts, but are semantically
invalid, into service to mean "reset to default" 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 ask for
"reset to default", or add a distinct value to @tls_creds and
@tls_hostname. This commit implements the latter: add JSON null to
the values of @tls_creds and @tls_hostname, deprecate "".
Because we're so close to the 2.10 freeze, implement it in the
stupidest way possible: have qmp_migrate_set_parameters() rewrite null
to "" before anything else can see the null. The proper way to do it
would be rewriting "" to null, but that requires fixing up code to
work with null. Add TODO comments for that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hmp.c | 8 ++++++--
migration/migration.c | 18 ++++++++++++++++--
qapi-schema.json | 11 +++++++++--
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/hmp.c b/hmp.c
index 0a5de75..40ebeef 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1541,11 +1541,15 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
break;
case MIGRATION_PARAMETER_TLS_CREDS:
p->has_tls_creds = true;
- visit_type_str(v, param, &p->tls_creds, &err);
+ p->tls_creds = g_new0(StrOrNull, 1);
+ p->tls_creds->type = QTYPE_QSTRING;
+ visit_type_str(v, param, &p->tls_creds->u.s, &err);
break;
case MIGRATION_PARAMETER_TLS_HOSTNAME:
p->has_tls_hostname = true;
- visit_type_str(v, param, &p->tls_hostname, &err);
+ p->tls_hostname = g_new0(StrOrNull, 1);
+ p->tls_hostname->type = QTYPE_QSTRING;
+ visit_type_str(v, param, &p->tls_hostname->u.s, &err);
break;
case MIGRATION_PARAMETER_MAX_BANDWIDTH:
p->has_max_bandwidth = true;
diff --git a/migration/migration.c b/migration/migration.c
index f6a9443..e2cfb99 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -703,6 +703,20 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
"x_checkpoint_delay",
"is invalid, it should be positive");
}
+ /* TODO Rewrite "" to null instead */
+ if (params->has_tls_creds
+ && params->tls_creds->type == QTYPE_QNULL) {
+ QDECREF(params->tls_creds->u.n);
+ params->tls_creds->type = QTYPE_QSTRING;
+ params->tls_creds->u.s = strdup("");
+ }
+ /* TODO Rewrite "" to null instead */
+ if (params->has_tls_hostname
+ && params->tls_hostname->type == QTYPE_QNULL) {
+ QDECREF(params->tls_hostname->u.n);
+ params->tls_hostname->type = QTYPE_QSTRING;
+ params->tls_hostname->u.s = strdup("");
+ }
/*
* TODO if we fuse MigrateSetParameters back into
@@ -726,11 +740,11 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
}
if (params->has_tls_creds) {
g_free(s->parameters.tls_creds);
- s->parameters.tls_creds = g_strdup(params->tls_creds);
+ s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
}
if (params->has_tls_hostname) {
g_free(s->parameters.tls_hostname);
- s->parameters.tls_hostname = g_strdup(params->tls_hostname);
+ s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
}
if (params->has_max_bandwidth) {
s->parameters.max_bandwidth = params->max_bandwidth;
diff --git a/qapi-schema.json b/qapi-schema.json
index b5ec942..247af24 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -116,6 +116,13 @@
{ 'command': 'qmp_capabilities' }
##
+# @StrOrNull:
+##
+{ 'alternate': 'StrOrNull',
+ 'data': { 's': 'str',
+ 'n': 'null' } }
+
+##
# @LostTickPolicy:
#
# Policy for handling lost ticks in timer devices.
@@ -1098,8 +1105,8 @@
'*decompress-threads': 'int',
'*cpu-throttle-initial': 'int',
'*cpu-throttle-increment': 'int',
- '*tls-creds': 'str',
- '*tls-hostname': 'str',
+ '*tls-creds': 'StrOrNull',
+ '*tls-hostname': 'StrOrNull',
'*max-bandwidth': 'int',
'*downtime-limit': 'int',
'*x-checkpoint-delay': 'int',
--
2.7.5
On Tue, Jul 18, 2017 at 03:41:26PM +0200, Markus Armbruster wrote: > migrate-set-parameters sets migration parameters according to is > arguments like this: > > * Present means "set the parameter to this value" > > * Absent means "leave the parameter unchanged" > > * Except for parameters tls_creds and tls_hostname, "" means "reset > the parameter to its default value > > The first two are perfectly normal: presence of the parameter makes > the command do something. > > 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 neither a valid TLS credentials ID, nor a valid > host name. > > Pressing argument values the schema accepts, but are semantically > invalid, into service to mean "reset to default" 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 ask for > "reset to default", or add a distinct value to @tls_creds and > @tls_hostname. This commit implements the latter: add JSON null to > the values of @tls_creds and @tls_hostname, deprecate "". > > Because we're so close to the 2.10 freeze, implement it in the > stupidest way possible: have qmp_migrate_set_parameters() rewrite null > to "" before anything else can see the null. The proper way to do it > would be rewriting "" to null, but that requires fixing up code to > work with null. Add TODO comments for that. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hmp.c | 8 ++++++-- > migration/migration.c | 18 ++++++++++++++++-- > qapi-schema.json | 11 +++++++++-- > 3 files changed, 31 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrange <berrange@redhat.com> 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 :|
On 07/18/2017 08:41 AM, Markus Armbruster wrote:
> migrate-set-parameters sets migration parameters according to is
> arguments like this:
>
> * Present means "set the parameter to this value"
>
> * Absent means "leave the parameter unchanged"
>
> * Except for parameters tls_creds and tls_hostname, "" means "reset
> the parameter to its default value
>
> The first two are perfectly normal: presence of the parameter makes
> the command do something.
>
> 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 neither a valid TLS credentials ID, nor a valid
> host name.
>
> Pressing argument values the schema accepts, but are semantically
> invalid, into service to mean "reset to default" 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 ask for
> "reset to default", or add a distinct value to @tls_creds and
> @tls_hostname. This commit implements the latter: add JSON null to
> the values of @tls_creds and @tls_hostname, deprecate "".
>
> Because we're so close to the 2.10 freeze, implement it in the
> stupidest way possible: have qmp_migrate_set_parameters() rewrite null
> to "" before anything else can see the null. The proper way to do it
> would be rewriting "" to null, but that requires fixing up code to
> work with null. Add TODO comments for that.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -116,6 +116,13 @@
> { 'command': 'qmp_capabilities' }
>
> ##
> +# @StrOrNull:
A little light on the documentation.
> +##
> +{ 'alternate': 'StrOrNull',
> + 'data': { 's': 'str',
> + 'n': 'null' } }
> +
> +##
> # @LostTickPolicy:
> #
> # Policy for handling lost ticks in timer devices.
> @@ -1098,8 +1105,8 @@
> '*decompress-threads': 'int',
> '*cpu-throttle-initial': 'int',
> '*cpu-throttle-increment': 'int',
> - '*tls-creds': 'str',
> - '*tls-hostname': 'str',
> + '*tls-creds': 'StrOrNull',
> + '*tls-hostname': 'StrOrNull',
> '*max-bandwidth': 'int',
> '*downtime-limit': 'int',
> '*x-checkpoint-delay': 'int',
By splitting input from output, you guarantee that 'null' won't appear
in the output and break an unsuspecting client, but that new clients can
introspect the existence of null and use that instead of "" on input.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 07/18/2017 08:41 AM, Markus Armbruster wrote:
>> migrate-set-parameters sets migration parameters according to is
>> arguments like this:
>>
>> * Present means "set the parameter to this value"
>>
>> * Absent means "leave the parameter unchanged"
>>
>> * Except for parameters tls_creds and tls_hostname, "" means "reset
>> the parameter to its default value
>>
>> The first two are perfectly normal: presence of the parameter makes
>> the command do something.
>>
>> 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 neither a valid TLS credentials ID, nor a valid
>> host name.
>>
>> Pressing argument values the schema accepts, but are semantically
>> invalid, into service to mean "reset to default" 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 ask for
>> "reset to default", or add a distinct value to @tls_creds and
>> @tls_hostname. This commit implements the latter: add JSON null to
>> the values of @tls_creds and @tls_hostname, deprecate "".
>>
>> Because we're so close to the 2.10 freeze, implement it in the
>> stupidest way possible: have qmp_migrate_set_parameters() rewrite null
>> to "" before anything else can see the null. The proper way to do it
>> would be rewriting "" to null, but that requires fixing up code to
>> work with null. Add TODO comments for that.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> +++ b/qapi-schema.json
>> @@ -116,6 +116,13 @@
>> { 'command': 'qmp_capabilities' }
>>
>> ##
>> +# @StrOrNull:
>
> A little light on the documentation.
>
>> +##
>> +{ 'alternate': 'StrOrNull',
>> + 'data': { 's': 'str',
>> + 'n': 'null' } }
>> +
>> +##
>> # @LostTickPolicy:
>> #
>> # Policy for handling lost ticks in timer devices.
>> @@ -1098,8 +1105,8 @@
>> '*decompress-threads': 'int',
>> '*cpu-throttle-initial': 'int',
>> '*cpu-throttle-increment': 'int',
>> - '*tls-creds': 'str',
>> - '*tls-hostname': 'str',
>> + '*tls-creds': 'StrOrNull',
>> + '*tls-hostname': 'StrOrNull',
>> '*max-bandwidth': 'int',
>> '*downtime-limit': 'int',
>> '*x-checkpoint-delay': 'int',
>
> By splitting input from output, you guarantee that 'null' won't appear
> in the output and break an unsuspecting client, but that new clients can
> introspect the existence of null and use that instead of "" on input.
Yes.
We don't want null in output. My unsharing makes that syntactically
impossible. Nice bonus, not why I went that route. I did because it
reduces code churn quite a bit, which is what we need right now.
Likewise, we don't want absent members in output. The sharing (commit
de63ab6) made that syntactically possible. My unsharing doesn't make it
impossible, because it refrains from removing the '*' from
MigrationParameters.
If we decide not to restore the sharing, we should make absent members
syntactically impossible.
If we decice to restore the sharing, null in output will become
syntactically possible.
We'll have to pick our poison: tighter schema or more concise schema.
We picked concise last time.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
Eric Blake <eblake@redhat.com> writes:
> On 07/18/2017 08:41 AM, Markus Armbruster wrote:
>> migrate-set-parameters sets migration parameters according to is
>> arguments like this:
>>
>> * Present means "set the parameter to this value"
>>
>> * Absent means "leave the parameter unchanged"
>>
>> * Except for parameters tls_creds and tls_hostname, "" means "reset
>> the parameter to its default value
>>
>> The first two are perfectly normal: presence of the parameter makes
>> the command do something.
>>
>> 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 neither a valid TLS credentials ID, nor a valid
>> host name.
>>
>> Pressing argument values the schema accepts, but are semantically
>> invalid, into service to mean "reset to default" 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 ask for
>> "reset to default", or add a distinct value to @tls_creds and
>> @tls_hostname. This commit implements the latter: add JSON null to
>> the values of @tls_creds and @tls_hostname, deprecate "".
>>
>> Because we're so close to the 2.10 freeze, implement it in the
>> stupidest way possible: have qmp_migrate_set_parameters() rewrite null
>> to "" before anything else can see the null. The proper way to do it
>> would be rewriting "" to null, but that requires fixing up code to
>> work with null. Add TODO comments for that.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> +++ b/qapi-schema.json
>> @@ -116,6 +116,13 @@
>> { 'command': 'qmp_capabilities' }
>>
>> ##
>> +# @StrOrNull:
>
> A little light on the documentation.
Care to suggest improvements? I figure the schema is obvious enough
without any, but the generated documentation could perhaps use some.
[...]
On 07/18/2017 01:39 PM, Markus Armbruster wrote:
>>> +++ b/qapi-schema.json
>>> @@ -116,6 +116,13 @@
>>> { 'command': 'qmp_capabilities' }
>>>
>>> ##
>>> +# @StrOrNull:
>>
>> A little light on the documentation.
>
> Care to suggest improvements? I figure the schema is obvious enough
> without any, but the generated documentation could perhaps use some.
Perhaps:
Specifies a string value or the explicit lack of a string (often used as
an optional parameter type, where omitting the parameter has different
semantics than supplying null).
@s a JSON string
@n an explicit NULL
and certainly worth having
Since: 2.10
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 07/18/2017 01:39 PM, Markus Armbruster wrote:
>
>>>> +++ b/qapi-schema.json
>>>> @@ -116,6 +116,13 @@
>>>> { 'command': 'qmp_capabilities' }
>>>>
>>>> ##
>>>> +# @StrOrNull:
>>>
>>> A little light on the documentation.
>>
>> Care to suggest improvements? I figure the schema is obvious enough
>> without any, but the generated documentation could perhaps use some.
>
> Perhaps:
>
> Specifies a string value or the explicit lack of a string (often used as
> an optional parameter type, where omitting the parameter has different
> semantics than supplying null).
> @s a JSON string
> @n an explicit NULL
>
>
> and certainly worth having
>
> Since: 2.10
What about:
##
# @StrOrNull:
#
# This is a string value or the explicit lack of a string (null
# pointer in C). Intended for cases when 'optional absent' already
# has a different meaning.
#
# @s: the string value
# @n: no string value
#
# Since: 2.10
##
On 07/18/2017 02:32 PM, Markus Armbruster wrote: > What about: > > ## > # @StrOrNull: > # > # This is a string value or the explicit lack of a string (null > # pointer in C). Intended for cases when 'optional absent' already > # has a different meaning. > # > # @s: the string value > # @n: no string value > # > # Since: 2.10 > ## Works for me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
* Markus Armbruster (armbru@redhat.com) wrote:
> migrate-set-parameters sets migration parameters according to is
> arguments like this:
>
> * Present means "set the parameter to this value"
>
> * Absent means "leave the parameter unchanged"
>
> * Except for parameters tls_creds and tls_hostname, "" means "reset
> the parameter to its default value
Is this really what's happening? IMHO the tls_creds and tls_hostname
behaviour isn't that "" resets to the default, it just is the default.
I don't think there's anything special cased for tls_creds and
tls_hostname in the existing code. It's this patch that's
adding more special casing.
(I'm not going to nack this, but I just don't get why it's such a big
deal)
Dave
> The first two are perfectly normal: presence of the parameter makes
> the command do something.
>
> 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 neither a valid TLS credentials ID, nor a valid
> host name.
>
> Pressing argument values the schema accepts, but are semantically
> invalid, into service to mean "reset to default" 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 ask for
> "reset to default", or add a distinct value to @tls_creds and
> @tls_hostname. This commit implements the latter: add JSON null to
> the values of @tls_creds and @tls_hostname, deprecate "".
>
> Because we're so close to the 2.10 freeze, implement it in the
> stupidest way possible: have qmp_migrate_set_parameters() rewrite null
> to "" before anything else can see the null. The proper way to do it
> would be rewriting "" to null, but that requires fixing up code to
> work with null. Add TODO comments for that.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hmp.c | 8 ++++++--
> migration/migration.c | 18 ++++++++++++++++--
> qapi-schema.json | 11 +++++++++--
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 0a5de75..40ebeef 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1541,11 +1541,15 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> break;
> case MIGRATION_PARAMETER_TLS_CREDS:
> p->has_tls_creds = true;
> - visit_type_str(v, param, &p->tls_creds, &err);
> + p->tls_creds = g_new0(StrOrNull, 1);
> + p->tls_creds->type = QTYPE_QSTRING;
> + visit_type_str(v, param, &p->tls_creds->u.s, &err);
> break;
> case MIGRATION_PARAMETER_TLS_HOSTNAME:
> p->has_tls_hostname = true;
> - visit_type_str(v, param, &p->tls_hostname, &err);
> + p->tls_hostname = g_new0(StrOrNull, 1);
> + p->tls_hostname->type = QTYPE_QSTRING;
> + visit_type_str(v, param, &p->tls_hostname->u.s, &err);
> break;
> case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> p->has_max_bandwidth = true;
> diff --git a/migration/migration.c b/migration/migration.c
> index f6a9443..e2cfb99 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -703,6 +703,20 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> "x_checkpoint_delay",
> "is invalid, it should be positive");
> }
> + /* TODO Rewrite "" to null instead */
> + if (params->has_tls_creds
> + && params->tls_creds->type == QTYPE_QNULL) {
> + QDECREF(params->tls_creds->u.n);
> + params->tls_creds->type = QTYPE_QSTRING;
> + params->tls_creds->u.s = strdup("");
> + }
> + /* TODO Rewrite "" to null instead */
> + if (params->has_tls_hostname
> + && params->tls_hostname->type == QTYPE_QNULL) {
> + QDECREF(params->tls_hostname->u.n);
> + params->tls_hostname->type = QTYPE_QSTRING;
> + params->tls_hostname->u.s = strdup("");
> + }
>
> /*
> * TODO if we fuse MigrateSetParameters back into
> @@ -726,11 +740,11 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> }
> if (params->has_tls_creds) {
> g_free(s->parameters.tls_creds);
> - s->parameters.tls_creds = g_strdup(params->tls_creds);
> + s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
> }
> if (params->has_tls_hostname) {
> g_free(s->parameters.tls_hostname);
> - s->parameters.tls_hostname = g_strdup(params->tls_hostname);
> + s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
> }
> if (params->has_max_bandwidth) {
> s->parameters.max_bandwidth = params->max_bandwidth;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b5ec942..247af24 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -116,6 +116,13 @@
> { 'command': 'qmp_capabilities' }
>
> ##
> +# @StrOrNull:
> +##
> +{ 'alternate': 'StrOrNull',
> + 'data': { 's': 'str',
> + 'n': 'null' } }
> +
> +##
> # @LostTickPolicy:
> #
> # Policy for handling lost ticks in timer devices.
> @@ -1098,8 +1105,8 @@
> '*decompress-threads': 'int',
> '*cpu-throttle-initial': 'int',
> '*cpu-throttle-increment': 'int',
> - '*tls-creds': 'str',
> - '*tls-hostname': 'str',
> + '*tls-creds': 'StrOrNull',
> + '*tls-hostname': 'StrOrNull',
> '*max-bandwidth': 'int',
> '*downtime-limit': 'int',
> '*x-checkpoint-delay': 'int',
> --
> 2.7.5
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> migrate-set-parameters sets migration parameters according to is
>> arguments like this:
>>
>> * Present means "set the parameter to this value"
>>
>> * Absent means "leave the parameter unchanged"
>>
>> * Except for parameters tls_creds and tls_hostname, "" means "reset
>> the parameter to its default value
>
> Is this really what's happening? IMHO the tls_creds and tls_hostname
> behaviour isn't that "" resets to the default, it just is the default.
> I don't think there's anything special cased for tls_creds and
> tls_hostname in the existing code. It's this patch that's
> adding more special casing.
>
> (I'm not going to nack this, but I just don't get why it's such a big
> deal)
It wouldn't call it a big deal. In fact, I called it a "minor QMP
interface design flaw".
The implementation encodes "no TLS credentials, do not use TLS" and "no
host name, fall back to the one in the migration URI" as "". Works
because "" is neither a valid TLS credentials ID, nor a valid host name.
Until commit 4af245d (v2.9.0), the implementation used NULL / optional
absent rather than "". Cleaner, because it doesn't rely on "" being
invalid.
The reason why commit 4af245d changed it to "" was that
migrate-set-parameters can't do NULL / optional absent. It can't,
because it already uses optional absent for "do not change this
parameter".
Replacing NULL by "" side-stepped this problem. I dislike that because
it's not general (only works as long as "" is not a valid value), and
ugly.
Instead of side-stepping the problem, I proposed to tackle it: make JSON
null mean NULL in migrate-set-parameters. However, the freeze was
literally the other day, we needed *some* solution, and only the one
making "" special was ready. So I let it pass.
Related: NULL used to be shown as "" in query-migrate-parameters.
Commit de63ab6 (v2.8.0) fixed that oddity, but commit 4af245d (v2.9.0)
regressed the fix, probably unintentionally.
My patch doesn't fix that regression. It doesn't revert the flip from
NULL to "". It merely corrects the QAPI interface, in the stupidest way
possible, because that's all I could do for 2.10's soft freeze.
I'm open to fixing the query-migrate-parameters regression during
freeze.
See also
Message-ID: <87379zhrhn.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html
and
Message-ID: <87bmt3t5r0.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02841.html
On Tue, Jul 18, 2017 at 09:24:08PM +0200, Markus Armbruster wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Markus Armbruster (armbru@redhat.com) wrote: > >> migrate-set-parameters sets migration parameters according to is > >> arguments like this: > >> > >> * Present means "set the parameter to this value" > >> > >> * Absent means "leave the parameter unchanged" > >> > >> * Except for parameters tls_creds and tls_hostname, "" means "reset > >> the parameter to its default value > > > > Is this really what's happening? IMHO the tls_creds and tls_hostname > > behaviour isn't that "" resets to the default, it just is the default. > > I don't think there's anything special cased for tls_creds and > > tls_hostname in the existing code. It's this patch that's > > adding more special casing. > > > > (I'm not going to nack this, but I just don't get why it's such a big > > deal) > > It wouldn't call it a big deal. In fact, I called it a "minor QMP > interface design flaw". > > The implementation encodes "no TLS credentials, do not use TLS" and "no > host name, fall back to the one in the migration URI" as "". Works > because "" is neither a valid TLS credentials ID, nor a valid host name. > > Until commit 4af245d (v2.9.0), the implementation used NULL / optional > absent rather than "". Cleaner, because it doesn't rely on "" being > invalid. > > The reason why commit 4af245d changed it to "" was that > migrate-set-parameters can't do NULL / optional absent. It can't, > because it already uses optional absent for "do not change this > parameter". > > Replacing NULL by "" side-stepped this problem. I dislike that because > it's not general (only works as long as "" is not a valid value), and > ugly. > > Instead of side-stepping the problem, I proposed to tackle it: make JSON > null mean NULL in migrate-set-parameters. However, the freeze was > literally the other day, we needed *some* solution, and only the one > making "" special was ready. So I let it pass. > > Related: NULL used to be shown as "" in query-migrate-parameters. > Commit de63ab6 (v2.8.0) fixed that oddity, but commit 4af245d (v2.9.0) > regressed the fix, probably unintentionally. From libvirt POV we considered de63ab6 to be the regression (from the original 2.6.0 behaviour), and 4af245d to be the fix for that regression. Libvirt expected to see "" for tls-hostname in the output - without that it doesn't know that tls-hostname is even a valida parameter. > I'm open to fixing the query-migrate-parameters regression during > freeze. No please don't. That will break libvirt yet again. 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 :|
© 2016 - 2026 Red Hat, Inc.