10:20 p.m.
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(a)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