5:46 p.m.
On 03/22/2017 12:26 PM, Jiri Denemark wrote:
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(a)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.
True, but that's not necessarily a correct approach *and* we've been
bitten by use after free before too. Since the VIR_FREE() operates on a
local variable, only this function would see migParams being set to NULL
- the caller though would not see that and thus (as in other cases) we
are forced to place a migParams = NULL; after a vir*Free() call. I
prefer this mechanism and quite frankly would like to see other
vir*Free() functions follow this, but I don't have the time or desire to
write that pile of patches.
> +{
> + 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.
I didn't want to see it lost as it's a really important distinction. I
will move into the struct. I disagree about having stuff like this in a
commit message. When I'm reading code - I'm not reading it as part of a
commit message, I'm reading it literally. The one concern I'd have
about moving it to the struct is someone not reading it...
John
> + 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