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

[Jiri Denemark]

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?

> +}
> +
> +
> +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.

>  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