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