On Thu, Feb 17, 2011 at 02:32:49PM +0800, Hu Tao wrote:

If we don't check it, virsh users will get "Disk detached successfully" even when qemuMonitorDriveDel fails. --- src/qemu/qemu_hotplug.c | 10 ++++++++-- src/qemu/qemu_monitor_json.c | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fb9db5a..70e9d8e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1187,7 +1187,10 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, }

/* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); + if (qemuMonitorDriveDel(priv->mon, drivestr) != 0) { + qemuDomainObjExitMonitor(vm); + goto cleanup; + }

qemuDomainObjExitMonitorWithDriver(driver, vm);

@@ -1269,7 +1272,10 @@ int qemuDomainDetachSCSIDiskDevice(struct qemud_driver *driver, }

/* disconnect guest from host device */ - qemuMonitorDriveDel(priv->mon, drivestr); + if (qemuMonitorDriveDel(priv->mon, drivestr) != 0) { + qemuDomainObjExitMonitor(vm); + goto cleanup; + }

This needs to be '< 0'. 0 indicates success, and '1' indicates that the command was not available in qemu which we need to treat as success too otherwise we cause a regression.

qemuDomainObjExitMonitorWithDriver(driver, vm);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d5e8d37..b088405 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2338,7 +2338,7 @@ int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, if (ret == 0) { /* See if drive_del isn't supported */ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - VIR_ERROR0(_("deleting disk is not supported. " + qemuReportError(VIR_ERR_NO_SUPPORT, _("deleting disk is not supported. " "This may leak data if disk is reassigned"));

NACK to this chunk. We deliberately do not report an error when the command is not found, because the 'drive_del' command does not exist in any released QEMU yet and we don't want to cause a regression on hotunplug for existing libvirt users.

ret = 1; goto cleanup;

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|