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@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 */
+ 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));
+}
+
+
+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 */
+ 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;
+}
+
+
static int
qemuDomainGetChardevTLSObjects(virQEMUDriverConfigPtr cfg,
qemuDomainObjPrivatePtr priv,
@@ -1582,8 +1665,6 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn,
char *charAlias = NULL;
char *devstr = NULL;
bool chardevAdded = false;
- bool tlsobjAdded = false;
- bool secobjAdded = false;
virJSONValuePtr tlsProps = NULL;
virJSONValuePtr secProps = NULL;
char *tlsAlias = NULL;
@@ -1619,25 +1700,11 @@ int qemuDomainAttachRedirdevDevice(virConnectPtr conn,
&secProps, &secAlias) < 0)
goto cleanup;
- qemuDomainObjEnterMonitor(driver, vm);
-
- if (secAlias) {
- rc = qemuMonitorAddObject(priv->mon, "secret",
- secAlias, secProps);
- secProps = NULL;
- if (rc < 0)
- goto exit_monitor;
- secobjAdded = true;
- }
+ if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps,
+ tlsAlias, &tlsProps) < 0)
+ goto cleanup;
- if (tlsAlias) {
- rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
- tlsAlias, tlsProps);
- tlsProps = NULL; /* qemuMonitorAddObject consumes */
- if (rc < 0)
- goto exit_monitor;
- tlsobjAdded = true;
- }
+ qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon,
charAlias,
@@ -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);
goto audit;
}
@@ -1858,10 +1922,8 @@ int qemuDomainAttachChrDevice(virConnectPtr conn,
virDomainChrSourceDefPtr dev = chr->source;
char *charAlias = NULL;
bool chardevAttached = false;
- bool tlsobjAdded = false;
bool teardowncgroup = false;
bool teardowndevice = false;
- bool secobjAdded = false;
virJSONValuePtr tlsProps = NULL;
char *tlsAlias = NULL;
virJSONValuePtr secProps = NULL;
@@ -1908,24 +1970,11 @@ int qemuDomainAttachChrDevice(virConnectPtr conn,
&secProps, &secAlias) < 0)
goto cleanup;
- qemuDomainObjEnterMonitor(driver, vm);
- if (secAlias) {
- rc = qemuMonitorAddObject(priv->mon, "secret",
- secAlias, secProps);
- secProps = NULL;
- if (rc < 0)
- goto exit_monitor;
- secobjAdded = true;
- }
+ if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps,
+ tlsAlias, &tlsProps) < 0)
+ goto cleanup;
- if (tlsAlias) {
- rc = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
- tlsAlias, tlsProps);
- tlsProps = NULL; /* qemuMonitorAddObject consumes */
- if (rc < 0)
- goto exit_monitor;
- tlsobjAdded = true;
- }
+ qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon, charAlias, chr->source) < 0)
goto exit_monitor;
@@ -1966,16 +2015,13 @@ int qemuDomainAttachChrDevice(virConnectPtr conn,
/* detach associated chardev on error */
if (chardevAttached)
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);
goto audit;
}
@@ -2000,8 +2046,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
bool teardowndevice = false;
bool chardevAdded = false;
bool objAdded = false;
- bool tlsobjAdded = false;
- bool secobjAdded = false;
virJSONValuePtr props = NULL;
virJSONValuePtr tlsProps = NULL;
virJSONValuePtr secProps = NULL;
@@ -2076,27 +2120,13 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
charAlias, &tlsProps, &tlsAlias,
&secProps, &secAlias) < 0)
goto cleanup;
- }
-
- qemuDomainObjEnterMonitor(driver, vm);
- if (secAlias) {
- rv = qemuMonitorAddObject(priv->mon, "secret",
- secAlias, secProps);
- secProps = NULL;
- if (rv < 0)
- goto exit_monitor;
- secobjAdded = true;
+ if (qemuDomainAddTLSObjects(driver, vm, secAlias, &secProps,
+ tlsAlias, &tlsProps) < 0)
+ goto cleanup;
}
- if (tlsAlias) {
- rv = qemuMonitorAddObject(priv->mon, "tls-creds-x509",
- tlsAlias, tlsProps);
- tlsProps = NULL; /* qemuMonitorAddObject consumes */
- if (rv < 0)
- goto exit_monitor;
- tlsobjAdded = true;
- }
+ qemuDomainObjEnterMonitor(driver, vm);
if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
qemuMonitorAttachCharDev(priv->mon, charAlias,
@@ -2152,10 +2182,6 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && 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);
@@ -2163,6 +2189,7 @@ qemuDomainAttachRNGDevice(virConnectPtr conn,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
releaseaddr = false;
+ qemuDomainDelTLSObjects(driver, vm, secAlias, tlsAlias);
goto audit;
}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 13242ee..c4f33e0 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -33,6 +33,19 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
virStorageSourcePtr newsrc,
bool force);
+
+void qemuDomainDelTLSObjects(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *secAlias,
+ const char *tlsAlias);
+
+int qemuDomainAddTLSObjects(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *secAlias,
+ virJSONValuePtr *secProps,
+ const char *tlsAlias,
+ virJSONValuePtr *tlsProps);
+
int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainControllerDefPtr controller);
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.