[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default |
Date: |
Tue, 18 Jul 2017 18:52:30 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
* Markus Armbruster (address@hidden) 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 <address@hidden>
> ---
> 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 / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH for-2.10 01/10] qapi: Separate type QNull from QObject, (continued)
- [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Markus Armbruster, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Daniel P. Berrange, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Eric Blake, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Markus Armbruster, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Markus Armbruster, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Eric Blake, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Markus Armbruster, 2017/07/18
- Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default, Eric Blake, 2017/07/18
Re: [Qemu-devel] [PATCH for-2.10 10/10] migration: Use JSON null instead of "" to reset parameter to default,
Dr. David Alan Gilbert <=
[Qemu-devel] [PATCH for-2.10 09/10] migration: Unshare MigrationParameters struct for now, Markus Armbruster, 2017/07/18
Re: [Qemu-devel] [PATCH for-2.10 00/10] Correct two minor QMP interface design flaws, no-reply, 2017/07/18
Re: [Qemu-devel] [PATCH for-2.10 00/10] Correct two minor QMP interface design flaws, Daniel P. Berrange, 2017/07/18