[libvirt] handling qemuMonitorAddDevice failure: missing drive_del function?
In src/qemu/qemu_driver.c, coverity gripes (rightly) about this: 6912 qemuDomainObjEnterMonitorWithDriver(driver, vm); 6913 if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { 6914 ret = qemuMonitorAddDrive(priv->mon, drivestr); 6915 if (ret == 0) No check of the return value of "qemuMonitorAddDevice(priv->mon, devstr)". Calling function "qemuMonitorAddDevice" without checking return value. 6916 qemuMonitorAddDevice(priv->mon, devstr); 6917 /* XXX remove the drive upon fail */ 6918 } else { Does anyone have a preference on how to deal with it while we wait for a drive-removal function? I think it deserves at least a diagnostic. I suppose this comment is still relevant: if (ret == 0) ret = qemuMonitorAddDevice(priv->mon, devstr); /* XXX should call 'drive_del' on error but this does not exist yet */ This XXX marks the same problem -- though note that coverity could not possibly see this one.
On Tue, May 18, 2010 at 03:23:23PM +0200, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity gripes (rightly) about this:
6912 qemuDomainObjEnterMonitorWithDriver(driver, vm); 6913 if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { 6914 ret = qemuMonitorAddDrive(priv->mon, drivestr); 6915 if (ret == 0) No check of the return value of "qemuMonitorAddDevice(priv->mon, devstr)". Calling function "qemuMonitorAddDevice" without checking return value. 6916 qemuMonitorAddDevice(priv->mon, devstr); 6917 /* XXX remove the drive upon fail */ 6918 } else {
Does anyone have a preference on how to deal with it while we wait for a drive-removal function? I think it deserves at least a diagnostic.
Add a VIR_WARN message i guess Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Tue, May 18, 2010 at 03:23:23PM +0200, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity gripes (rightly) about this:
6912 qemuDomainObjEnterMonitorWithDriver(driver, vm); 6913 if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { 6914 ret = qemuMonitorAddDrive(priv->mon, drivestr); 6915 if (ret == 0) No check of the return value of "qemuMonitorAddDevice(priv->mon, devstr)". Calling function "qemuMonitorAddDevice" without checking return value. 6916 qemuMonitorAddDevice(priv->mon, devstr); 6917 /* XXX remove the drive upon fail */ 6918 } else {
Does anyone have a preference on how to deal with it while we wait for a drive-removal function? I think it deserves at least a diagnostic.
Add a VIR_WARN message i guess
Here you go:
From 62ece506ead7534ac37a70a6750a97e69809d088 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 16:02:12 +0200 Subject: [PATCH] do not ignore qemuMonitorAddDrive failure; make uses identical
There were three very similar uses of qemuMonitorAddDrive. This change makes the three 17-line sequences identical. * src/qemu/qemu_driver.c (qemudDomainAttachPciDiskDevice): Detect failure. Add VIR_WARN and braces. (qemudDomainAttachSCSIDisk): Add VIR_WARN and braces. (qemudDomainAttachUsbMassstorageDevice): Likewise. --- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++--------------- 1 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5649a20..9d23191 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6912,15 +6912,21 @@ static int qemudDomainAttachPciDiskDevice(struct qemud_driver *driver, goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ret = qemuMonitorAddDrive(priv->mon, drivestr); - if (ret == 0) - qemuMonitorAddDevice(priv->mon, devstr); - /* XXX remove the drive upon fail */ + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { virDomainDevicePCIAddress guestAddr; ret = qemuMonitorAddPCIDisk(priv->mon, disk->src, type, &guestAddr); @@ -7126,18 +7132,22 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver, virReportOOMError(); goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ret = qemuMonitorAddDrive(priv->mon, - drivestr); - if (ret == 0) - ret = qemuMonitorAddDevice(priv->mon, - devstr); - /* XXX should call 'drive_del' on error but this does not exist yet */ + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { virDomainDeviceDriveAddress driveAddr; ret = qemuMonitorAttachDrive(priv->mon, drivestr, &cont->info.addr.pci, &driveAddr); @@ -7215,18 +7225,22 @@ static int qemudDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, virReportOOMError(); goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ret = qemuMonitorAddDrive(priv->mon, - drivestr); - if (ret == 0) - ret = qemuMonitorAddDevice(priv->mon, - devstr); - /* XXX should call 'drive_del' on error but this does not exist yet */ + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) -- 1.7.1.250.g7d1e8
On Tue, May 18, 2010 at 04:02:44PM +0200, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, May 18, 2010 at 03:23:23PM +0200, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity gripes (rightly) about this:
6912 qemuDomainObjEnterMonitorWithDriver(driver, vm); 6913 if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { 6914 ret = qemuMonitorAddDrive(priv->mon, drivestr); 6915 if (ret == 0) No check of the return value of "qemuMonitorAddDevice(priv->mon, devstr)". Calling function "qemuMonitorAddDevice" without checking return value. 6916 qemuMonitorAddDevice(priv->mon, devstr); 6917 /* XXX remove the drive upon fail */ 6918 } else {
Does anyone have a preference on how to deal with it while we wait for a drive-removal function? I think it deserves at least a diagnostic.
Add a VIR_WARN message i guess
Here you go:
From 62ece506ead7534ac37a70a6750a97e69809d088 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 16:02:12 +0200 Subject: [PATCH] do not ignore qemuMonitorAddDrive failure; make uses identical
There were three very similar uses of qemuMonitorAddDrive. This change makes the three 17-line sequences identical. * src/qemu/qemu_driver.c (qemudDomainAttachPciDiskDevice): Detect failure. Add VIR_WARN and braces. (qemudDomainAttachSCSIDisk): Add VIR_WARN and braces. (qemudDomainAttachUsbMassstorageDevice): Likewise. --- src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++--------------- 1 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5649a20..9d23191 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6912,15 +6912,21 @@ static int qemudDomainAttachPciDiskDevice(struct qemud_driver *driver, goto error; }
qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { ret = qemuMonitorAddDrive(priv->mon, drivestr); - if (ret == 0) - qemuMonitorAddDevice(priv->mon, devstr); - /* XXX remove the drive upon fail */ + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { virDomainDevicePCIAddress guestAddr; ret = qemuMonitorAddPCIDisk(priv->mon, disk->src, type, &guestAddr); @@ -7126,18 +7132,22 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver, virReportOOMError(); goto error; }
qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ret = qemuMonitorAddDrive(priv->mon, - drivestr); - if (ret == 0) - ret = qemuMonitorAddDevice(priv->mon, - devstr); - /* XXX should call 'drive_del' on error but this does not exist yet */ + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { virDomainDeviceDriveAddress driveAddr; ret = qemuMonitorAttachDrive(priv->mon, drivestr, &cont->info.addr.pci, &driveAddr); @@ -7215,18 +7225,22 @@ static int qemudDomainAttachUsbMassstorageDevice(struct qemud_driver *driver, virReportOOMError(); goto error; }
qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ret = qemuMonitorAddDrive(priv->mon, - drivestr); - if (ret == 0) - ret = qemuMonitorAddDevice(priv->mon, - devstr); - /* XXX should call 'drive_del' on error but this does not exist yet */ + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN(_("qemuMonitorAddDevice failed on %s (%s)"), + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + } } else { ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); } qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret < 0)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Jim Meyering