[libvirt] [PATCH 05/13] qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects

[John Ferlan]

On 02/21/2017 03:00 PM, Jiri Denemark wrote:
> On Fri, Feb 17, 2017 at 14:39:22 -0500, John Ferlan wrote:
>> Refactor the TLS object adding code to make two separate API's that will
>> handle the add/remove of the "secret" and "tls-creds-x509" objects including
>> the Enter/Exit monitor commands.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu_hotplug.c | 169 ++++++++++++++++++++++++++++--------------------
>>  src/qemu/qemu_hotplug.h |  13 ++++
>>  2 files changed, 111 insertions(+), 71 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 8d15eee..fb8a052 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1526,6 +1526,89 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
>>  }
>>  
>>  
>> +void
>> +qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
>> +                        virDomainObjPtr vm,
>> +                        const char *secAlias,
>> +                        const char *tlsAlias)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virErrorPtr orig_err;
>> +
>> +    /* Nothing to do if neither defined */
> 
> The comment is pretty redundant.
> 
>> +    if (!tlsAlias && !secAlias)
>> +        return;
>> +
>> +    orig_err = virSaveLastError();
>> +
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +    if (tlsAlias)
>> +        ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
>> +    if (secAlias)
>> +        ignore_value(qemuMonitorDelObject(priv->mon, secAlias));
>> +    if (orig_err) {
>> +        virSetError(orig_err);
>> +        virFreeError(orig_err);
>> +    }
>> +    ignore_value(qemuDomainObjExitMonitor(driver, vm));
> 
> It looks like this function is designed to preserve the original error,
> so shouldn't the call to ExitMonitor be done above the if (orig_err)
> statement?
> 

One would think that would be fine, but then again once you check each
of the places where I'm moving code the ExitMonitor is done after
resetting the orig_err.  IDC either way, but I was more or less
following current design.

>> +}
>> +
>> +
>> +int
>> +qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
>> +                        virDomainObjPtr vm,
>> +                        const char *secAlias,
>> +                        virJSONValuePtr *secProps,
>> +                        const char *tlsAlias,
>> +                        virJSONValuePtr *tlsProps)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    int rc;
>> +    bool secobjAdded = false;
>> +    bool tlsobjAdded = false;
>> +    virErrorPtr orig_err;
>> +
>> +    /* Nothing to do if neither defined */
> 
> Redundant comment again.
> 
>> +    if (!tlsAlias && !secAlias)
>> +        return 0;
>> +
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +    if (secAlias) {
>> +        rc = qemuMonitorAddObject(priv->mon, "secret",
>> +                                  secAlias, *secProps);
>> +        *secProps = NULL; /* qemuMonitorAddObject consumes */
>> +        if (rc < 0)
>> +            goto exit_monitor;
>> +        secobjAdded = true;
>> +    }
>> +
>> +    if (tlsAlias) {
>> +        rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
>> +                                  tlsAlias, *tlsProps);
>> +        *tlsProps = NULL; /* qemuMonitorAddObject consumes */
>> +        if (rc < 0)
>> +            goto exit_monitor;
>> +        tlsobjAdded = true;
>> +    }
>> +
>> +    return qemuDomainObjExitMonitor(driver, vm);
>> +
>> + exit_monitor:
>> +    orig_err = virSaveLastError();
>> +    if (tlsobjAdded)
>> +        ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
>> +    if (secobjAdded)
>> +        ignore_value(qemuMonitorDelObject(priv->mon, secAlias));
>> +    if (orig_err) {
>> +        virSetError(orig_err);
>> +        virFreeError(orig_err);
>> +    }
>> +    ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +    return -1;
> 
> This is suspicious, you're open coding almost all of the DelTLSObjects
> here. Could this function be rewritten to make use of
> the just introduced qemuDomainDelTLSObjects? Adding an extra ExitMonitor
> followed by EnterMonitor if something failed shouldn't be a big deal.
> We're entering and exiting the monitor all the time anyway.
> 

Yeah I can do this - probably just call the ExitMonitor first and then
the DelTLSObjects afterwards which will re-enter and delete.


Tks, -

John
>>  static int
>>  qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg,
>>                                 qemuDomainObjPrivatePtr priv,
> ...
>> @@ -1672,15 +1739,12 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn,
>>      /* detach associated chardev on error */
>>      if (chardevAdded)
>>          ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
>> -    if (tlsobjAdded)
>> -        ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
>> -    if (secobjAdded)
>> -        ignore_value(qemuMonitorDelObject(priv->mon, secAlias));
>>      if (orig_err) {
>>          virSetError(orig_err);
>>          virFreeError(orig_err);
>>      }
>>      ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +    qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
> 
> OK, we get here only when both tls and sec objects were added.
> 
>>      goto audit;
> 
> BTW, the audit label can be easily replaced with cleanup. And this
> likely applies to the two clones (qemuDomainAttachChrDevice,
> qemuDomainAttachRNGDevice) of this function too.
> 
> Jirka
>