[libvirt] [PATCH 0/7] Error path cleanup for hotplug and one function move/rename
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
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@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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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
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@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
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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
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
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
participants (2)
-
John Ferlan -
Ján Tomko