Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

[Eric Blake]

On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> Some of the migration parameters are strings, which default to NULL,
> eg tls_hostname and tls_creds.
> 
> The mgmt app will set the tls_creds parameter on both source and target
> QEMU instances, in order to trigger use of TLS for migration.
> 
> After performing a TLS encrypted migration though, migration might be
> used for other reasons - for example, to save the QEMU state to a file.
> We need TLS turned off when doing this, but the migrate-set-parameters
> QAPI command does not provide any facility to clear/reset parameters
> to their default state.
> 
> If you simply ommit the tls_creds parameter in migrate-set-parameters,

s/ommit/omit/

> then 'has_tls_creds' will be false and so no action will be taken. The
> only option that works with migrate-set-parameters is to treat "" on
> the wire as equivalent to requesting NULL. Failing that we would have
> to create a new 'migrate-reset-parameters' method to explicitly put
> a parameter back to its default value.

That happens to work in this case, because an empty string as the TLS
parameter makes no sense.

A nicer solution would be to support JSON null on the wire, so the user
can pass "tls-creds":null to explicitly request wiping out the
credentials rather than making the empty string magic.  But that's more
invasive (the QAPI parser would have to be enhanced to allow an
alternate that visits either a string or null).  Ultimately, I think
we'll have to do that when we come up with some string property where ""
should not be given magic treatment.

So now we have a dilemma: do we special case "" here, to be stuck with
it even if we later add nullable-string support later, or do we go all
the way to nullable-string support now?  We've missed soft freeze for
2.9, so my vote is that it is okay to special case "" in  this instance,
even though we'll have to continue to support it indefinitely even if we
gain nullable-string QMP.

> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> 
> CC'ing Eric & Markus since this is related to QAPI schema for migrate
> monitor commands
> 

>      }
>      if (params->has_tls_creds) {
>          g_free(s->parameters.tls_creds);
> -        s->parameters.tls_creds = g_strdup(params->tls_creds);
> +        if (*params->tls_creds == '\0') {
> +            s->parameters.tls_creds = NULL;

I'm wondering if you should also do s->parameters.has_tls_creds = false
at this point?  The visitors expect that if has_tls_creds is true, then
the string is non-NULL.

> +        } else {
> +            s->parameters.tls_creds = g_strdup(params->tls_creds);
> +        }
>      }
>      if (params->has_tls_hostname) {
>          g_free(s->parameters.tls_hostname);
> -        s->parameters.tls_hostname = g_strdup(params->tls_hostname);
> +        if (*params->tls_hostnane == '\0') {
> +            s->parameters.tls_hostnane = NULL;

This build-breaking typo isn't intended. :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature