[libvirt] [PATCH v3 4/7] qemu: Add TLS params to _qemuMonitorMigrationParams

[John Ferlan]

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 at 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
>