[libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams

[Jiri Denemark]

On Fri, Mar 17, 2017 at 14:38:58 -0400, John Ferlan wrote:
> Add the fields to support setting tls-creds and tls-hostname during
> a migration (either source or target). Modify the query migration
> function to check for the presence and set the field for future
> consumers to determine which of 3 conditions is being met (not
> present, present and set to "", or present and sent to something).
> 
> Modify code paths that either allocate or use stack space in order
> to call qemuMigrationParamsClear or qemuMigrationParamsFree for cleanup.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/qemu/qemu_driver.c       |  4 +++-
>  src/qemu/qemu_migration.c    | 26 +++++++++++++++++++++++++-
>  src/qemu/qemu_migration.h    |  6 ++++++
>  src/qemu/qemu_monitor.c      | 11 ++++++++---
>  src/qemu/qemu_monitor.h      |  3 +++
>  src/qemu/qemu_monitor_json.c | 28 ++++++++++++++++++++++++++++
>  tests/qemumonitorjsontest.c  | 25 ++++++++++++++++++++++++-
>  7 files changed, 97 insertions(+), 6 deletions(-)
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index f5711bc..66a5062 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3508,6 +3508,28 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
>  }
>  
>  
> +void
> +qemuMigrationParamsClear(qemuMonitorMigrationParamsPtr migParams)
> +{
> +    if (!migParams)
> +        return;
> +
> +    VIR_FREE(migParams->migrateTLSAlias);
> +    VIR_FREE(migParams->migrateTLSHostname);
> +}
> +
> +
> +void
> +qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams)

Our *Free functions don't usually get double pointers.

> +{
> +    if (!*migParams)
> +        return;
> +
> +    qemuMigrationParamsClear(*migParams);
> +    VIR_FREE(*migParams);
> +}
> +
> +
>  qemuMonitorMigrationParamsPtr
>  qemuMigrationParams(virTypedParameterPtr params,
>                      int nparams,
...
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 553544a..125cc6a 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
...
> @@ -2595,6 +2596,21 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
>  
>  #undef PARSE
>  
> +    /* NB: First supported in QEMU 2.7; however, there was no way to
> +     * clear, so 2.9 altered the definition to allow using an empty string
> +     * to disable. Additionally, it defined the variable to an empty string
> +     * by default if not defined ever. Use this as our marker to determine
> +     * whether TLS can be supported or not. */

This comment could make some sense in the commit message (unlike
describing which paths are changed by the patch), but I don't think it's
any useful here. Describing that NULL means unsupported and "" means
unset would be enough I think. And even better if this is documented
inside struct _qemuMonitorMigrationParams.

> +    if ((tlsStr = virJSONValueObjectGetString(result, "tls-creds"))) {
> +        if (VIR_STRDUP(params->migrateTLSAlias, tlsStr) < 0)
> +            goto cleanup;
> +    }
> +
> +    if ((tlsStr = virJSONValueObjectGetString(result, "tls-hostname"))) {
> +        if (VIR_STRDUP(params->migrateTLSHostname, tlsStr) < 0)
> +            goto cleanup;
> +    }
> +
>      ret = 0;
>   cleanup:
>      virJSONValueFree(cmd);
> @@ -2637,6 +2653,18 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
>  
>  #undef APPEND
>  
> +    /* See query, value will be either NULL, "", or something valid.
> +     * NULL will indicate no support, while "" will indicate to disable */

Yeah, this is what I was thinking about (except for the "See query"
part). And I still think it would make sense to move it to struct
_qemuMonitorMigrationParams.

> +    if (params->migrateTLSAlias &&
> +        virJSONValueObjectAppendString(args, "tls-creds",
> +                                       params->migrateTLSAlias) < 0)
> +        goto cleanup;
> +
> +    if (params->migrateTLSHostname &&
> +        virJSONValueObjectAppendString(args, "tls-hostname",
> +                                       params->migrateTLSHostname) < 0)
> +        goto cleanup;
> +
>      if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
>          goto cleanup;
>      args = NULL;

Jirka