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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
