Re: [libvirt] [PATCH 10/13] qemu: Add TLS params to _qemuMonitorMigrationParams

On Tue, Feb 21, 2017 at 22:06:02 +0100, Jiri Denemark wrote: > On Fri, Feb 17, 2017 at 14:39:27 -0500, John Ferlan wrote: >> Add the fields to support setting tls-creds and tls-hostname during >> a migration (either source or target) >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/qemu/qemu_monitor.c | 12 +++++++++--- >> src/qemu/qemu_monitor.h | 7 +++++++ >> src/qemu/qemu_monitor_json.c | 11 +++++++++++ >> 3 files changed, 27 insertions(+), 3 deletions(-) > ... >> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >> index 8811d85..d719112 100644 >> --- a/src/qemu/qemu_monitor.h >> +++ b/src/qemu/qemu_monitor.h >> @@ -570,6 +570,13 @@ struct _qemuMonitorMigrationParams { >> >> bool cpuThrottleIncrement_set; >> int cpuThrottleIncrement; >> + >> + /* Input only for destination */ > > What do you mean by this comment? I think you can just safely drop it > :-) >

>> + bool migrateTLSAlias_set; >> + char *migrateTLSAlias; >> + >> + bool migrateTLSHostname_set; >> + char *migrateTLSHostname; > > Both parameters are set-only, we never read them back from QEMU so > there's no need for the *_set booleans. Especially when NULL tells that > pretty clearly. > >> }; >> >> int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >> index 7aa9e31..7a70366 100644 >> --- a/src/qemu/qemu_monitor_json.c >> +++ b/src/qemu/qemu_monitor_json.c >> @@ -2637,6 +2637,17 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, >> >> #undef APPEND >> >> + /* Set only parameters for TLS migration options */ > > Looks like another useless comment. > >> + if (params->migrateTLSAlias_set && >> + virJSONValueObjectAppendString(args, "tls-creds", >> + params->migrateTLSAlias) < 0) >> + goto cleanup; >> + >> + if (params->migrateTLSHostname_set && >> + virJSONValueObjectAppendString(args, "tls-hostname", >> + params->migrateTLSHostname) < 0) >> + goto cleanup; And we should always set these parameters if QEMU supports them to make sure some stale values are not used when VIR_MIGRATE_TLS is not set. So we might actually need the *_set booleans, but it depends on the way we implement the logic to reset the parameters. Which brings a question... how do we reset these parameters?