[libvirt] handling qemuMonitorAddDevice failure: missing drive_del function?

[Daniel P. Berrange]

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 at 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 :|