8:50 p.m.
During review of the LUKS series:
http://www.redhat.com/archives/libvir-list/2016-July/msg00488.html
it was noted that the error paths should use bools rather than goto failxxx
type labels. So this series does that for a few of the affect APIs. There's
also changes to qemuDomainAttachMemory since it seems it wasn't properly
handling 'props' at least w/r/t how qemuDomainAttachRNGDevice would clean
things up.
Finally - another part of the review noted that the qemu command line
comma escaping could be made more available since the storage driver
would potentially need it to build the qemuimg command line. So I moved
to virqemu and renamed.
John Ferlan (7):
qemu: Alter error path cleanup for qemuDomainAttachHostSCSIDevice
qemu: Alter error path cleanup for qemuDomainAttachVirtioDiskDevice
qemu: Alter error path cleanup for qemuDomainAttachSCSIDisk
qemu: Alter error path cleanup for qemuDomainAttachChrDevice
qemu: Alter error path cleanup for qemuDomainAttachRNGDevice
qemu: Alter error path cleanup for qemuDomainAttachMemory
qemu: Move and rename qemuBufferEscapeComma
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 33 +++-------
src/qemu/qemu_hotplug.c | 163 +++++++++++++++++++++++++++--------------------
src/util/virqemu.c | 15 +++++
src/util/virqemu.h | 2 +
5 files changed, 122 insertions(+), 92 deletions(-)
--
2.5.5
8:50 p.m.
New subject: [libvirt] [PATCH 1/7] qemu: Alter error path cleanup for qemuDomainAttachHostSCSIDevice
Based on recent review comment - rather than have a spate of goto failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 04e11b4..10120aa 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1874,6 +1874,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
char *drvstr = NULL;
bool teardowncgroup = false;
bool teardownlabel = false;
+ bool cleanupAddDrive = false;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1935,17 +1936,17 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
goto cleanup;
- /* Attach the device - 2 step process */
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAddDrive(priv->mon, drvstr) < 0)
- goto failadddrive;
+ goto monitor_error;
+ cleanupAddDrive = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddevice;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto failexitmonitor;
+ goto cleanup;
virDomainAuditHostdev(vm, hostdev, "attach", true);
@@ -1968,21 +1969,18 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
VIR_FREE(devstr);
return ret;
- failadddevice:
+ monitor_error:
orig_err = virSaveLastError();
- if (qemuMonitorDriveDel(priv->mon, drvstr) < 0)
+ if (cleanupAddDrive && qemuMonitorDriveDel(priv->mon, drvstr) < 0) {
VIR_WARN("Unable to remove drive %s (%s) after failed "
"qemuMonitorAddDevice",
drvstr, devstr);
+ }
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
}
-
- failadddrive:
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
-
- failexitmonitor:
virDomainAuditHostdev(vm, hostdev, "attach", false);
goto cleanup;
--
2.5.5
7:32 p.m.
New subject: [libvirt] [PATCH 1/7] qemu: Alter error path cleanup for qemuDomainAttachHostSCSIDevice
On Fri, Jul 15, 2016 at 07:50:21AM -0400, John Ferlan wrote:
Based on recent review comment - rather than have a spate of goto
failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 04e11b4..10120aa 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1874,6 +1874,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
char *drvstr = NULL;
bool teardowncgroup = false;
bool teardownlabel = false;
+ bool cleanupAddDrive = false;
driveAdded is shorter
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1935,17 +1936,17 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
goto cleanup;
- /* Attach the device - 2 step process */
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAddDrive(priv->mon, drvstr) < 0)
- goto failadddrive;
+ goto monitor_error;
+ cleanupAddDrive = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddevice;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto failexitmonitor;
+ goto cleanup;
virDomainAuditHostdev(vm, hostdev, "attach", true);
@@ -1968,21 +1969,18 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
VIR_FREE(devstr);
return ret;
- failadddevice:
+ monitor_error:
can be 'exit_monitor' to match what we already use.
orig_err = virSaveLastError();
- if (qemuMonitorDriveDel(priv->mon, drvstr) < 0)
+ if (cleanupAddDrive && qemuMonitorDriveDel(priv->mon, drvstr) < 0) {
VIR_WARN("Unable to remove drive %s (%s) after failed "
"qemuMonitorAddDevice",
drvstr, devstr);
+ }
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
The error reported by qemuDomainObjExitMonitor ('domain is no longer
running') could be more important than the original error.
I would leave it where it was.
Also, it's possible that ExitMonitor can no longer fail now that we take
a job for cleaning up after EOF.
ACK either way.
Jan
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
}
-
- failadddrive:
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
-
- failexitmonitor:
virDomainAuditHostdev(vm, hostdev, "attach", false);
goto cleanup;
--
2.5.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:50 p.m.
New subject: [libvirt] [PATCH 2/7] qemu: Alter error path cleanup for qemuDomainAttachVirtioDiskDevice
Based on recent review comment - rather than have a spate of goto failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 10120aa..ce1db3c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -308,6 +308,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
char *drivestr = NULL;
char *drivealias = NULL;
bool releaseaddr = false;
+ bool cleanupAddDrive = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
const char *src = virDomainDiskGetSource(disk);
@@ -354,18 +355,18 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
goto error;
- /* Attach the device - 2 step process */
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
- goto failadddrive;
+ goto monitor_error;
+ cleanupAddDrive = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddevice;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
releaseaddr = false;
- goto failexitmonitor;
+ goto error;
}
virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
@@ -381,22 +382,19 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
virObjectUnref(cfg);
return ret;
- failadddevice:
+ monitor_error:
orig_err = virSaveLastError();
- if (qemuMonitorDriveDel(priv->mon, drivealias) < 0) {
+ if (cleanupAddDrive && qemuMonitorDriveDel(priv->mon, drivealias) < 0)
{
VIR_WARN("Unable to remove drive %s (%s) after failed "
"qemuMonitorAddDevice", drivealias, drivestr);
}
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ releaseaddr = false;
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
}
- failadddrive:
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- releaseaddr = false;
-
- failexitmonitor:
virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
error:
--
2.5.5
8:50 p.m.
New subject: [libvirt] [PATCH 3/7] qemu: Alter error path cleanup for qemuDomainAttachSCSIDisk
Based on recent review comment - rather than have a spate of goto failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ce1db3c..0fc0084 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -544,8 +544,10 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
{
size_t i;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
char *drivestr = NULL;
char *devstr = NULL;
+ bool cleanupAddDrive = false;
int ret = -1;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -587,17 +589,17 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
goto error;
- /* Attach the device - 2 step process */
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
- goto failadddrive;
+ goto monitor_error;
+ cleanupAddDrive = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddevice;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto failexitmonitor;
+ goto error;
virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
@@ -611,14 +613,18 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
virObjectUnref(cfg);
return ret;
- failadddevice:
+ monitor_error:
+ orig_err = virSaveLastError();
/* XXX should call 'drive_del' on error but this does not exist yet */
- VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr);
+ if (cleanupAddDrive)
+ VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr);
- failadddrive:
ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
+ }
- failexitmonitor:
virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
error:
--
2.5.5
7:34 p.m.
New subject: [libvirt] [PATCH 3/7] qemu: Alter error path cleanup for qemuDomainAttachSCSIDisk
On Fri, Jul 15, 2016 at 07:50:23AM -0400, John Ferlan wrote:
Based on recent review comment - rather than have a spate of goto
failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ce1db3c..0fc0084 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -544,8 +544,10 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
{
size_t i;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
char *drivestr = NULL;
char *devstr = NULL;
+ bool cleanupAddDrive = false;
int ret = -1;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -587,17 +589,17 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
goto error;
- /* Attach the device - 2 step process */
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
- goto failadddrive;
+ goto monitor_error;
+ cleanupAddDrive = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddevice;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
- goto failexitmonitor;
+ goto error;
virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
@@ -611,14 +613,18 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn,
virObjectUnref(cfg);
return ret;
- failadddevice:
+ monitor_error:
+ orig_err = virSaveLastError();
/* XXX should call 'drive_del' on error but this does not exist yet */
- VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr);
+ if (cleanupAddDrive)
+ VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr);
- failadddrive:
ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
+ }
I don't think we need to save the error just for ExitMonitor
Jan
- failexitmonitor:
virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
error:
--
2.5.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:50 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Alter error path cleanup for qemuDomainAttachChrDevice
Based on recent review comment - rather than have a spate of goto failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0fc0084..a220d9f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1514,9 +1514,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
{
int ret = -1, rc;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
virDomainDefPtr vmdef = vm->def;
char *devstr = NULL;
char *charAlias = NULL;
+ bool cleanupAttachCharDev = false;
bool need_release = false;
if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
@@ -1542,10 +1544,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0)
- goto failchardev;
+ goto monitor_error;
+ cleanupAttachCharDev = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failadddev;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto audit;
@@ -1563,11 +1566,17 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
VIR_FREE(devstr);
return ret;
- failadddev:
+ monitor_error:
+ orig_err = virSaveLastError();
/* detach associated chardev on error */
- qemuMonitorDetachCharDev(priv->mon, charAlias);
- failchardev:
+ if (cleanupAttachCharDev)
+ qemuMonitorDetachCharDev(priv->mon, charAlias);
+
ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
+ }
goto audit;
}
--
2.5.5
7:35 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Alter error path cleanup for qemuDomainAttachChrDevice
On Fri, Jul 15, 2016 at 07:50:24AM -0400, John Ferlan wrote:
Based on recent review comment - rather than have a spate of goto
failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0fc0084..a220d9f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1514,9 +1514,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
{
int ret = -1, rc;
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
virDomainDefPtr vmdef = vm->def;
char *devstr = NULL;
char *charAlias = NULL;
+ bool cleanupAttachCharDev = false;
bool need_release = false;
if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
@@ -1542,10 +1544,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0)
- goto failchardev;
+ goto monitor_error;
+ cleanupAttachCharDev = true;
chardevAttached?
Jan
8:50 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Alter error path cleanup for qemuDomainAttachRNGDevice
Based on recent review comment - rather than have a spate of goto failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a220d9f..c5a1a91 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1587,12 +1587,17 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainRNGDefPtr rng)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
char *devstr = NULL;
char *charAlias = NULL;
char *objAlias = NULL;
+ bool releaseaddr = false;
+ bool cleanupCharAlias = false;
+ bool cleanupObjAlias = false;
virJSONValuePtr props = NULL;
const char *type;
int ret = -1;
+ int rv;
if (qemuAssignDeviceRNGAlias(vm->def, rng) < 0)
return -1;
@@ -1613,6 +1618,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
rng->source.file))
return -1;
}
+ releaseaddr = true;
if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
@@ -1637,23 +1643,25 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0)
goto cleanup;
- /* attach the device - up to a 3 stage process */
qemuDomainObjEnterMonitor(driver, vm);
if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
qemuMonitorAttachCharDev(priv->mon, charAlias,
rng->source.chardev) < 0)
- goto failchardev;
+ goto monitor_error;
+ cleanupCharAlias = true;
- if (qemuMonitorAddObject(priv->mon, type, objAlias, props) < 0)
- goto failbackend;
- props = NULL;
+ rv = qemuMonitorAddObject(priv->mon, type, objAlias, props);
+ props = NULL; /* qemuMonitorAddObject consumes */
+ if (rv < 0)
+ goto monitor_error;
+ cleanupObjAlias = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failfrontend;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- vm = NULL;
+ releaseaddr = false;
goto cleanup;
}
@@ -1665,24 +1673,24 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0);
cleanup:
virJSONValueFree(props);
- if (ret < 0 && vm)
+ if (ret < 0 && releaseaddr)
qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL);
VIR_FREE(charAlias);
VIR_FREE(objAlias);
VIR_FREE(devstr);
return ret;
- /* rollback */
- failfrontend:
- ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
- failbackend:
- if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
+ monitor_error:
+ orig_err = virSaveLastError();
+ if (cleanupObjAlias)
+ ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && cleanupCharAlias)
ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
- props = NULL; /* qemuMonitorAddObject consumes on failure */
- failchardev:
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- vm = NULL;
- goto cleanup;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ releaseaddr = false;
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
}
goto audit;
--
2.5.5
7:38 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Alter error path cleanup for qemuDomainAttachRNGDevice
On Fri, Jul 15, 2016 at 07:50:25AM -0400, John Ferlan wrote:
Based on recent review comment - rather than have a spate of goto
failxxxx,
change to a boolean based model. Ensures that the original error can be
preserved and cleanup is a bit more orderly if more objects are added.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 44 ++++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a220d9f..c5a1a91 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1587,12 +1587,17 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainRNGDefPtr rng)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
char *devstr = NULL;
char *charAlias = NULL;
char *objAlias = NULL;
+ bool releaseaddr = false;
+ bool cleanupCharAlias = false;
could be chardevAdded
+ bool cleanupObjAlias = false;
objAdded
virJSONValuePtr props = NULL;
const char *type;
int ret = -1;
+ int rv;
if (qemuAssignDeviceRNGAlias(vm->def, rng) < 0)
return -1;
@@ -1613,6 +1618,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
rng->source.file))
return -1;
}
+ releaseaddr = true;
if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
@@ -1637,23 +1643,25 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0)
goto cleanup;
- /* attach the device - up to a 3 stage process */
qemuDomainObjEnterMonitor(driver, vm);
if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD &&
qemuMonitorAttachCharDev(priv->mon, charAlias,
rng->source.chardev) < 0)
- goto failchardev;
+ goto monitor_error;
+ cleanupCharAlias = true;
- if (qemuMonitorAddObject(priv->mon, type, objAlias, props) < 0)
- goto failbackend;
- props = NULL;
+ rv = qemuMonitorAddObject(priv->mon, type, objAlias, props);
+ props = NULL; /* qemuMonitorAddObject consumes */
+ if (rv < 0)
+ goto monitor_error;
+ cleanupObjAlias = true;
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
- goto failfrontend;
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- vm = NULL;
+ releaseaddr = false;
goto cleanup;
}
@@ -1665,24 +1673,24 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0);
cleanup:
virJSONValueFree(props);
- if (ret < 0 && vm)
+ if (ret < 0 && releaseaddr)
qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL);
VIR_FREE(charAlias);
VIR_FREE(objAlias);
VIR_FREE(devstr);
return ret;
- /* rollback */
- failfrontend:
- ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
- failbackend:
- if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD)
+ monitor_error:
exit_monitor is nicer IMO.
+ orig_err = virSaveLastError();
+ if (cleanupObjAlias)
+ ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
+ if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && cleanupCharAlias)
ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
- props = NULL; /* qemuMonitorAddObject consumes on failure */
- failchardev:
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
- vm = NULL;
- goto cleanup;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
Same comment about the error applies.
Jan
+ releaseaddr = false;
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
}
goto audit;
--
2.5.5
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:50 p.m.
New subject: [libvirt] [PATCH 6/7] qemu: Alter error path cleanup for qemuDomainAttachMemory
A recent adjustment to qemuDomainAttachRNGDevice to properly cleanup
the props object after a qemuMonitorAddObject also would affect this
code. Alter the cleanup to be similar to RNG changes.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c5a1a91..7cc52e7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1713,16 +1713,19 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
virDomainMemoryDefPtr mem)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
+ virErrorPtr orig_err;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
unsigned long long newmem = oldmem + mem->size;
char *devstr = NULL;
char *objalias = NULL;
const char *backendType;
+ bool cleanupObjAlias = false;
virJSONValuePtr props = NULL;
virObjectEventPtr event;
int id;
int ret = -1;
+ int rv;
qemuDomainMemoryDeviceAlignSize(vm->def, mem);
@@ -1755,16 +1758,14 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
}
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorAddObject(priv->mon, backendType, objalias, props) < 0)
- goto exit_monitor;
+ rv = qemuMonitorAddObject(priv->mon, backendType, objalias, props);
+ props = NULL; /* qemuMonitorAddObject consumes */
+ if (rv < 0)
+ goto monitor_error;
+ cleanupObjAlias = true;
- if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
- virErrorPtr err = virSaveLastError();
- ignore_value(qemuMonitorDelObject(priv->mon, objalias));
- virSetError(err);
- virFreeError(err);
- goto exit_monitor;
- }
+ if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+ goto monitor_error;
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
/* we shouldn't touch mem now, as the def might be freed */
@@ -1796,11 +1797,18 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
virDomainMemoryDefFree(mem);
return ret;
- exit_monitor:
- if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ monitor_error:
+ orig_err = virSaveLastError();
+ if (cleanupObjAlias)
+ ignore_value(qemuMonitorDelObject(priv->mon, objalias));
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
mem = NULL;
- goto audit;
+ if (orig_err) {
+ virSetError(orig_err);
+ virFreeError(orig_err);
}
+ if (!mem)
+ goto audit;
removedef:
if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
@@ -1809,10 +1817,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
mem = NULL;
/* reset the mlock limit */
- virErrorPtr err = virSaveLastError();
+ orig_err = virSaveLastError();
ignore_value(qemuDomainAdjustMaxMemLock(vm));
- virSetError(err);
- virFreeError(err);
+ virSetError(orig_err);
+ virFreeError(orig_err);
goto audit;
}
--
2.5.5
8:50 p.m.
New subject: [libvirt] [PATCH 7/7] qemu: Move and rename qemuBufferEscapeComma
Move to virqemu and rename to virQEMUBuildBufferEscapeComma. This can then
be shared by the storage driver when it builds the command line for qemuimg
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 33 +++++++++------------------------
src/util/virqemu.c | 15 +++++++++++++++
src/util/virqemu.h | 2 ++
4 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ba718b8..dcbff06 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2192,6 +2192,7 @@ virProcessWait;
# util/virqemu.h
+virQEMUBuildBufferEscapeComma;
virQEMUBuildObjectCommandlineFromJSON;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fe4bb88..ba50987 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -152,21 +152,6 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST,
/**
- * qemuBufferEscapeComma:
- * @buf: buffer to append the escaped string
- * @str: the string to escape
- *
- * qemu requires that any values passed on the command line which contain
- * a ',' must escape it using an extra ',' as the escape character
- */
-static void
-qemuBufferEscapeComma(virBufferPtr buf, const char *str)
-{
- virBufferEscape(buf, ',', ",", "%s", str);
-}
-
-
-/**
* qemuBuildHasMasterKey:
* @qemuCaps: QEMU binary capabilities
*
@@ -221,7 +206,7 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, "-object");
virBufferAsprintf(&buf, "secret,id=%s,format=raw,file=", alias);
- qemuBufferEscapeComma(&buf, path);
+ virQEMUBuildBufferEscapeComma(&buf, path);
virCommandAddArgBuffer(cmd, &buf);
ret = 0;
@@ -1229,7 +1214,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
break;
}
- qemuBufferEscapeComma(&opt, source);
+ virQEMUBuildBufferEscapeComma(&opt, source);
virBufferAddLit(&opt, ",");
if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
@@ -3773,7 +3758,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
goto error;
}
virBufferAsprintf(&buf, "virtio-input-host%s,id=%s,evdev=", suffix,
dev->info.alias);
- qemuBufferEscapeComma(&buf, dev->source.evdev);
+ virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev);
break;
case VIR_DOMAIN_INPUT_TYPE_LAST:
break;
@@ -4775,7 +4760,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
case VIR_DOMAIN_CHR_TYPE_UNIX:
virBufferAsprintf(&buf, "socket,id=char%s,path=", alias);
- qemuBufferEscapeComma(&buf, dev->data.nix.path);
+ virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path);
if (dev->data.nix.listen)
virBufferAddLit(&buf, ",server,nowait");
break;
@@ -6147,7 +6132,7 @@ qemuBuildBootCommandLine(virCommandPtr cmd,
virBuffer buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-acpitable");
virBufferAddLit(&buf, "sig=SLIC,file=");
- qemuBufferEscapeComma(&buf, def->os.slic_table);
+ virQEMUBuildBufferEscapeComma(&buf, def->os.slic_table);
virCommandAddArgBuffer(cmd, &buf);
}
@@ -6734,7 +6719,7 @@ qemuBuildNameCommandLine(virCommandPtr cmd,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_GUEST))
virBufferAddLit(&buf, "guest=");
- qemuBufferEscapeComma(&buf, def->name);
+ virQEMUBuildBufferEscapeComma(&buf, def->name);
if (cfg->setProcessName &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_PROCESS))
@@ -7271,7 +7256,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
switch (glisten->type) {
case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
virBufferAddLit(&opt, "unix:");
- qemuBufferEscapeComma(&opt, glisten->socket);
+ virQEMUBuildBufferEscapeComma(&opt, glisten->socket);
break;
case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
@@ -8847,10 +8832,10 @@ qemuBuildTPMBackendStr(const virDomainDef *def,
goto error;
}
virBufferAddLit(&buf, ",path=");
- qemuBufferEscapeComma(&buf, devset ? devset : tpmdev);
+ virQEMUBuildBufferEscapeComma(&buf, devset ? devset : tpmdev);
virBufferAddLit(&buf, ",cancel-path=");
- qemuBufferEscapeComma(&buf, cancel_path);
+ virQEMUBuildBufferEscapeComma(&buf, cancel_path);
VIR_FREE(devset);
VIR_FREE(cancel_path);
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 895168e..79a4292 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -140,3 +140,18 @@ virQEMUBuildObjectCommandlineFromJSON(const char *type,
virBufferFreeAndReset(&buf);
return ret;
}
+
+
+/**
+ * virQEMUBuildBufferEscapeComma:
+ * @buf: buffer to append the escaped string
+ * @str: the string to escape
+ *
+ * qemu requires that any values passed on the command line which contain
+ * a ',' must escape it using an extra ',' as the escape character
+ */
+void
+virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str)
+{
+ virBufferEscape(buf, ',', ",", "%s", str);
+}
diff --git a/src/util/virqemu.h b/src/util/virqemu.h
index af4ec1d..1033412 100644
--- a/src/util/virqemu.h
+++ b/src/util/virqemu.h
@@ -25,10 +25,12 @@
# define __VIR_QEMU_H_
# include "internal.h"
+# include "virbuffer.h"
# include "virjson.h"
char *virQEMUBuildObjectCommandlineFromJSON(const char *type,
const char *alias,
virJSONValuePtr props);
+void virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str);
#endif /* __VIR_QEMU_H_ */
--
2.5.5
7:40 p.m.
On Fri, Jul 15, 2016 at 07:50:20AM -0400, John Ferlan wrote:
During review of the LUKS series:
http://www.redhat.com/archives/libvir-list/2016-July/msg00488.html
it was noted that the error paths should use bools rather than goto failxxx
type labels. So this series does that for a few of the affect APIs. There's
also changes to qemuDomainAttachMemory since it seems it wasn't properly
handling 'props' at least w/r/t how qemuDomainAttachRNGDevice would clean
things up.
Finally - another part of the review noted that the qemu command line
comma escaping could be made more available since the storage driver
would potentially need it to build the qemuimg command line. So I moved
to virqemu and renamed.
John Ferlan (7):
qemu: Alter error path cleanup for qemuDomainAttachHostSCSIDevice
qemu: Alter error path cleanup for qemuDomainAttachVirtioDiskDevice
qemu: Alter error path cleanup for qemuDomainAttachSCSIDisk
qemu: Alter error path cleanup for qemuDomainAttachChrDevice
qemu: Alter error path cleanup for qemuDomainAttachRNGDevice
qemu: Alter error path cleanup for qemuDomainAttachMemory
qemu: Move and rename qemuBufferEscapeComma
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 33 +++-------
src/qemu/qemu_hotplug.c | 163 +++++++++++++++++++++++++++--------------------
src/util/virqemu.c | 15 +++++
src/util/virqemu.h | 2 +
5 files changed, 122 insertions(+), 92 deletions(-)
ACK series, regardless of the suggestions.
Jan
8:21 p.m.
On 07/19/2016 06:40 AM, Ján Tomko wrote:
On Fri, Jul 15, 2016 at 07:50:20AM -0400, John Ferlan wrote:
> During review of the LUKS series:
>
> http://www.redhat.com/archives/libvir-list/2016-July/msg00488.html
>
> it was noted that the error paths should use bools rather than goto
> failxxx
> type labels. So this series does that for a few of the affect APIs.
> There's
> also changes to qemuDomainAttachMemory since it seems it wasn't properly
> handling 'props' at least w/r/t how qemuDomainAttachRNGDevice would clean
> things up.
>
> Finally - another part of the review noted that the qemu command line
> comma escaping could be made more available since the storage driver
> would potentially need it to build the qemuimg command line. So I moved
> to virqemu and renamed.
>
> John Ferlan (7):
> qemu: Alter error path cleanup for qemuDomainAttachHostSCSIDevice
> qemu: Alter error path cleanup for qemuDomainAttachVirtioDiskDevice
> qemu: Alter error path cleanup for qemuDomainAttachSCSIDisk
> qemu: Alter error path cleanup for qemuDomainAttachChrDevice
> qemu: Alter error path cleanup for qemuDomainAttachRNGDevice
> qemu: Alter error path cleanup for qemuDomainAttachMemory
> qemu: Move and rename qemuBufferEscapeComma
>
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 33 +++-------
> src/qemu/qemu_hotplug.c | 163
> +++++++++++++++++++++++++++--------------------
> src/util/virqemu.c | 15 +++++
> src/util/virqemu.h | 2 +
> 5 files changed, 122 insertions(+), 92 deletions(-)
ACK series, regardless of the suggestions.
I altered the bool's to use shorter names, used exit_monitor, and moved
the qemuDomainObjExitMonitor outside the orig_err save for each.
Removed the save error for the qemuDomainAttachSCSIDisk cleanup
(although I have other code which will replace it with the drive delete
command too!).
Series now pushed - thanks!
John
3149
days inactive
3153
days old
13 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko