Re: [libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams
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
>> ---
>> 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 &&
>> +
Re: [libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams
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
> ---
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams
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
---
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_driver.c b/src/qemu/qemu_driver.c
index dcd823f..03e3f8d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11845,6 +11845,7 @@ qemuDomainMigratePerform(virDomainPtr dom,
flags, dname, resource, false);
cleanup:
+qemuMigrationParamsClear(&migParams);
VIR_FREE(compression);
return ret;
}
@@ -12253,6 +12254,7 @@ qemuDomainMigratePerform3(virDomainPtr dom,
flags, dname, resource, true);
cleanup:
+qemuMigrationParamsClear(&migParams);
VIR_FREE(compression);
return ret;
}
@@ -12343,7 +12345,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom,
flags, dname, bandwidth, true);
cleanup:
VIR_FREE(compression);
-VIR_FREE(migParams);
+qemuMigrationParamsFree(&migParams);
VIR_FREE(migrate_disks);
return ret;
}
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)
+{
+if (!*migParams)
+return;
+
+qemuMigrationParamsClear(*migParams);
+VIR_FREE(*migParams);
+}
+
+
qemuMonitorMigrationParamsPtr
qemuMigrationParams(virTypedParameterPtr params,
int nparams,
@@ -3549,7 +3571,7 @@ qemuMigrationParams(virTypedParameterPtr params,
return migParams;
error:
-VIR_FREE(migParams);
+qemuMigrationParamsFree(&migParams);
return NULL;
}
@@ -3909,6 +3931,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
virDomainObjRemoveTransientDef(vm);
qemuDomainRemoveInactive(driver, vm);
}
+qemuMigrationParamsClear(&migParams);
virDomainObjEndAPI(&vm);
qemuDomainEventQueue(driver, event);
qemuMigrationCookieFree(mig);
@@ -5244,6 +5267,7 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
virSetError(orig_err);
virFreeError(orig_err);
}
+qemuMigrationParamsClear(&migParams);
VIR_FREE(uri_out);
VIR_FREE(cookie);
VIR_FREE(compression);
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index bcebf06..4c8f2c9 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -121,6 +121,12 @@ int
qemuMigrationCompressionDump(qemuMigrationCompressionPtr compression,
int *maxparams,
unsigned long *flags);
+void
+qemuMigrationParamsClear(qemuMonitorMigrationParamsPtr migParams);
+
+void
+qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams);
+
qemuMonitorMigrationParamsPtr
qemuMigrationParams(virTypedParameterPtr params,
int nparams,
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 79da472..ee0e116 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2530,12 +2530,15 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
{
VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
"decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
- "cpuThrottleIncrement=%d:%d",
+ "cpuThrottleIncrement=%d:%d tlsAlias=%s "
+ "tlsHostname=%s",
params->compressLevel_set, params->compressLevel,
params->compressThreads_set, params->compressThreads,
params->decompressThreads_set, params->decompressThreads,
params->cpuThrottleInitial_set, params->cpuThrottleInitial,
- params->cpuThrottleIncrement_set, params->cpuThrottleIncrement);
+ params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
+ NULLSTR(params->migrateTLSAlias),
+
