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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
