Properly support SCSI drive hotplug

The current SCSI hotplug support attaches a brand new SCSI controller
for every disk. This is broken because the semantics differ from those
used when starting the VM initially. In the latter case, each SCSI
controller is filled before a new one is added.

If the user specifies an high drive index (sdazz) then at initial
startup, many intermediate SCSI controllers may be added with no
drives.

This patch changes SCSI hotplug so that it exactly matches the
behaviour of initial startup. First the SCSI controller number is
determined for the drive to be hotplugged. If any controller upto
and including that controller number is not yet present, it is
attached. Then finally the drive is attached to the last controller.

NB, this breaks SCSI hotunplug, because there is no 'drive_del'
command in current QEMU. Previous SCSI hotunplug was broken in
any case because it was unplugging the entire controller, not
just the drive in question.

A future QEMU will allow proper SCSI hotunplug of a drive.

This patch is derived from work done by Wolfgang Mauerer on disk
controllers.

* src/qemu/qemu_driver.c: Fix SCSI hotplug to add a drive to
 the correct controller, instead of just attaching a new
  controller.
* src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h,
  src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
  src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add
  support for 'drive_add' command

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 54769611a43bb537238d4b8e375a11b93b9d790d..39267e8e86d258c9888b18da594540756b3f9acc 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -185,6 +185,7 @@ virDomainDeviceInfoIsSet;
 virDomainDeviceAddressClear;
 virDomainControllerTypeToString;
 virDomainControllerDefFree;
+virDomainDeviceAddressTypeToString;
 
 
 # domain_event.h

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3d5e5e089759753e67a11eeead633177c98e0740..3d43571403fdaf84b73ecc09dda394a18bc65b36 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5170,6 +5170,116 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn,
     return ret;
 }
 
+static virDomainControllerDefPtr
+qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn,
+                                         struct qemud_driver *driver,
+                                         virDomainObjPtr vm,
+                                         int controller)
+{
+    int i;
+    virDomainControllerDefPtr cont;
+    for (i = 0 ; i < vm->def->ncontrollers ; i++) {
+        cont = vm->def->controllers[i];
+
+        if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+            continue;
+
+        if (cont->idx == controller)
+            return cont;
+    }
+
+    /* No SCSI controller present, for back compatability we
+     * now hotplug a controller */
+    if (VIR_ALLOC(cont) < 0) {
+        virReportOOMError(conn);
+        return NULL;
+    }
+    cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
+    cont->idx = 0;
+
+    VIR_INFO0("No SCSI controller present, hotplugging one");
+    if (qemudDomainAttachPciControllerDevice(conn, driver,
+                                             vm, cont) < 0) {
+        VIR_FREE(cont);
+        return NULL;
+    }
+    return cont;
+}
+
+static int qemudDomainAttachSCSIDisk(virConnectPtr conn,
+                                     struct qemud_driver *driver,
+                                     virDomainObjPtr vm,
+                                     virDomainDiskDefPtr dev,
+                                     int qemuCmdFlags)
+{
+    int i;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    virDomainDeviceDriveAddress driveAddr;
+    virDomainControllerDefPtr cont;
+    char *drivestr = NULL;
+    int ret = -1;
+
+    for (i = 0 ; i < vm->def->ndisks ; i++) {
+        if (STREQ(vm->def->disks[i]->dst, dev->dst)) {
+            qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                           _("target %s already exists"), dev->dst);
+            goto cleanup;
+        }
+    }
+
+    /* This func allocates the bus/unit IDs so must be before
+     * we search for controller
+     */
+    if (!(drivestr = qemuBuildDriveStr(dev, 0, qemuCmdFlags)))
+        goto cleanup;
+
+
+    /* We should have an adddress now, so make sure */
+    if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("unexpected disk address type %s"),
+                         virDomainDeviceAddressTypeToString(dev->info.type));
+        goto cleanup;
+    }
+
+    for (i = 0 ; i < dev->info.addr.drive.controller ; i++) {
+        cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i);
+        if (!cont)
+            goto cleanup;
+    }
+
+    if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+        qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("SCSI controller %d was missing its PCI address"), cont->idx);
+        goto cleanup;
+    }
+
+    if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
+        virReportOOMError(conn);
+        goto cleanup;
+    }
+
+    qemuDomainObjEnterMonitorWithDriver(driver, vm);
+    ret = qemuMonitorAttachDrive(priv->mon,
+                                 drivestr,
+                                 &cont->info.addr.pci,
+                                 &driveAddr);
+
+    qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+    if (ret == 0) {
+        /* XXX we should probably validate that the addr matches
+         * our existing defined addr instead of overwriting */
+        dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+        memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr));
+        virDomainDiskInsertPreAlloced(vm->def, dev);
+    }
+
+cleanup:
+    VIR_FREE(drivestr);
+    return ret;
+}
+
 
 static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn,
                                                  struct qemud_driver *driver,
@@ -5535,9 +5645,10 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
 
             if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
                 ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev);
-            } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI ||
-                       dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
+            } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
                 ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev);
+            } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+                ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm, dev->data.disk, qemuCmdFlags);
             } else {
                 qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                                  _("disk bus '%s' cannot be hotplugged."),
@@ -5959,8 +6070,7 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
 
     if (dev->type == VIR_DOMAIN_DEVICE_DISK &&
         dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
-        (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI ||
-         dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) {
+        dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
         ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
         if (driver->securityDriver)
             driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk);
@@ -5979,9 +6089,10 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
         }
     } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
         ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
-    } else
+    } else {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
-                         "%s", _("only SCSI or virtio disk device can be detached dynamically"));
+                         "%s", _("This type of device cannot be hot unplugged"));
+    }
 
     if (!ret && virDomainSaveStatus(dom->conn, driver->caps, driver->stateDir, vm) < 0)
         ret = -1;

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dca89068a7ed631202809aadc4404c126c93c18b..b6ffc2656e75b749112ff9d83a42895a2b0804a9 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1266,3 +1266,23 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
 
     return ret;
 }
+
+
+int qemuMonitorAttachDrive(qemuMonitorPtr mon,
+                           const char *drivestr,
+                           virDomainDevicePCIAddress *controllerAddr,
+                           virDomainDeviceDriveAddress *driveAddr)
+{
+    DEBUG("mon=%p, fd=%d drivestr=%s domain=%d bus=%d slot=%d function=%d",
+          mon, mon->fd, drivestr,
+          controllerAddr->domain, controllerAddr->bus,
+          controllerAddr->slot, controllerAddr->function);
+    int ret;
+
+    if (mon->json)
+        ret = qemuMonitorJSONAttachDrive(mon, drivestr, controllerAddr, driveAddr);
+    else
+        ret = qemuMonitorTextAttachDrive(mon, drivestr, controllerAddr, driveAddr);
+
+    return ret;
+}

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1096106b93454d8a55708213878b94809044deba..e270299688569e0f0c1f30197812bfa624a38841 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -269,4 +269,9 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
                                        const char *bus,
                                        virDomainDevicePCIAddress *guestAddr);
 
+int qemuMonitorAttachDrive(qemuMonitorPtr mon,
+                           const char *drivestr,
+                           virDomainDevicePCIAddress *controllerAddr,
+                           virDomainDeviceDriveAddress *driveAddr);
+
 #endif /* QEMU_MONITOR_H */

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e2bd1b1b9c9b1a08b4ac7918f87539eaa354bff1..0ae8cd61f019147d60a76cdff98ac47c81910865 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1204,8 +1204,8 @@ int qemuMonitorJSONAddUSBDeviceMatch(qemuMonitorPtr mon,
 
 
 static int
-qemuMonitorJSONGetGuestAddress(virJSONValuePtr reply,
-                               virDomainDevicePCIAddress *guestAddr)
+qemuMonitorJSONGetGuestPCIAddress(virJSONValuePtr reply,
+                                  virDomainDevicePCIAddress *guestAddr)
 {
     virJSONValuePtr addr;
 
@@ -1277,7 +1277,7 @@ int qemuMonitorJSONAddPCIHostDevice(qemuMonitorPtr mon,
         ret = qemuMonitorJSONCheckError(cmd, reply);
 
     if (ret == 0 &&
-        qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0)
+        qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0)
         ret = -1;
 
     virJSONValueFree(cmd);
@@ -1318,7 +1318,7 @@ int qemuMonitorJSONAddPCIDisk(qemuMonitorPtr mon,
         ret = qemuMonitorJSONCheckError(cmd, reply);
 
     if (ret == 0 &&
-        qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0)
+        qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0)
         ret = -1;
 
     virJSONValueFree(cmd);
@@ -1350,7 +1350,7 @@ int qemuMonitorJSONAddPCINetwork(qemuMonitorPtr mon,
         ret = qemuMonitorJSONCheckError(cmd, reply);
 
     if (ret == 0 &&
-        qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0)
+        qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0)
         ret = -1;
 
     virJSONValueFree(cmd);
@@ -1514,7 +1514,75 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
         ret = qemuMonitorJSONCheckError(cmd, reply);
 
     if (ret == 0 &&
-        qemuMonitorJSONGetGuestAddress(reply, guestAddr) < 0)
+        qemuMonitorJSONGetGuestPCIAddress(reply, guestAddr) < 0)
+        ret = -1;
+
+    virJSONValueFree(cmd);
+    virJSONValueFree(reply);
+    return ret;
+}
+
+
+static int
+qemuMonitorJSONGetGuestDriveAddress(virJSONValuePtr reply,
+                                    virDomainDeviceDriveAddress *driveAddr)
+{
+    virJSONValuePtr addr;
+
+    addr = virJSONValueObjectGet(reply, "return");
+    if (!addr || addr->type != VIR_JSON_TYPE_OBJECT) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("drive_add reply was missing device address"));
+        return -1;
+    }
+
+    if (virJSONValueObjectGetNumberUint(addr, "bus", &driveAddr->bus) < 0) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("drive_add reply was missing device bus number"));
+        return -1;
+    }
+
+    if (virJSONValueObjectGetNumberUint(addr, "unit", &driveAddr->unit) < 0) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("drive_add reply was missing device unit number"));
+        return -1;
+    }
+
+    return 0;
+}
+
+
+int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon,
+                               const char *drivestr,
+                               virDomainDevicePCIAddress* controllerAddr,
+                               virDomainDeviceDriveAddress* driveAddr)
+{
+    int ret;
+    virJSONValuePtr cmd = NULL;
+    virJSONValuePtr reply = NULL;
+    char *dev;
+
+    if (virAsprintf(&dev, "%.2x:%.2x.%.1x",
+                    controllerAddr->bus, controllerAddr->slot, controllerAddr->function) < 0) {
+        virReportOOMError(NULL);
+        return -1;
+    }
+
+    cmd = qemuMonitorJSONMakeCommand("drive_add",
+                                     "s:pci_addr", dev,
+                                     "s:opts", drivestr,
+                                     NULL);
+    VIR_FREE(dev);
+    if (!cmd)
+        return -1;
+
+    ret = qemuMonitorJSONCommand(mon, cmd, &reply);
+
+    if (ret == 0)
+        ret = qemuMonitorJSONCheckError(cmd, reply);
+
+    if (ret == 0 &&
+        qemuMonitorJSONGetGuestDriveAddress(reply, driveAddr) < 0)
         ret = -1;
 
     virJSONValueFree(cmd);

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 96eb68dc3b39dbb847998d552468f1b460c24ec3..fcab48338960a568a24925ebb1afbaba5b861b45 100644 (file)
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -145,4 +145,9 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
                                            const char *bus,
                                            virDomainDevicePCIAddress *guestAddr);
 
+int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon,
+                               const char *drivestr,
+                               virDomainDevicePCIAddress *controllerAddr,
+                               virDomainDeviceDriveAddress *driveAddr);
+
 #endif /* QEMU_MONITOR_JSON_H */

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index db3912e86d1ab8fef211b984d46dc85581700403..c2c37b41781a4130cacc6da7a6d46bf0794682ec 100644 (file)
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1795,3 +1795,105 @@ cleanup:
     VIR_FREE(reply);
     return ret;
 }
+
+
+static int
+qemudParseDriveAddReply(const char *reply,
+                        virDomainDeviceDriveAddressPtr addr)
+{
+    char *s, *e;
+
+    /* If the command succeeds qemu prints:
+     * OK bus X, unit Y
+     */
+
+    if (!(s = strstr(reply, "OK ")))
+        return -1;
+
+    s += 3;
+
+    if (STRPREFIX(s, "bus ")) {
+        s += strlen("bus ");
+
+        if (virStrToLong_ui(s, &e, 10, &addr->bus) == -1) {
+            VIR_WARN(_("Unable to parse bus '%s'\n"), s);
+            return -1;
+        }
+
+        if (!STRPREFIX(e, ", ")) {
+            VIR_WARN(_("Expected ', ' parsing drive_add reply '%s'\n"), s);
+            return -1;
+        }
+        s = e + 2;
+    }
+
+    if (!STRPREFIX(s, "unit ")) {
+        VIR_WARN(_("Expected 'unit ' parsing drive_add reply '%s'\n"), s);
+        return -1;
+    }
+    s += strlen("bus ");
+
+    if (virStrToLong_ui(s, &e, 10, &addr->unit) == -1) {
+        VIR_WARN(_("Unable to parse unit number '%s'\n"), s);
+        return -1;
+    }
+
+    return 0;
+}
+
+
+int qemuMonitorTextAttachDrive(qemuMonitorPtr mon,
+                               const char *drivestr,
+                               virDomainDevicePCIAddress *controllerAddr,
+                               virDomainDeviceDriveAddress *driveAddr)
+{
+    char *cmd = NULL;
+    char *reply = NULL;
+    int ret = -1;
+    char *safe_str;
+    int tryOldSyntax = 0;
+
+    safe_str = qemuMonitorEscapeArg(drivestr);
+    if (!safe_str) {
+        virReportOOMError(NULL);
+        return -1;
+    }
+
+try_command:
+    ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x %s",
+                      (tryOldSyntax ? "" : "pci_addr="),
+                      controllerAddr->domain, controllerAddr->bus,
+                      controllerAddr->slot, safe_str);
+    if (ret == -1) {
+        virReportOOMError(NULL);
+        goto cleanup;
+    }
+
+    if (qemuMonitorCommand(mon, cmd, &reply) < 0) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+                         _("failed to close fd in qemu with '%s'"), cmd);
+        goto cleanup;
+    }
+
+    if (qemudParseDriveAddReply(reply, driveAddr) < 0) {
+        if (!tryOldSyntax && strstr(reply, "invalid char in expression")) {
+            VIR_FREE(reply);
+            tryOldSyntax = 1;
+            goto try_command;
+        }
+        qemudReportError (NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+                          _("adding %s disk failed: %s"), drivestr, reply);
+        VIR_FREE(reply);
+        return -1;
+    }
+
+    ret = 0;
+
+cleanup:
+    VIR_FREE(cmd);
+    VIR_FREE(reply);
+    VIR_FREE(safe_str);
+    return ret;
+}
+
+

diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index ca2538ad761e53aadf780c2808cdbf8cd0bd6b4b..4648ba15c092dceeb46c7a332eb722c520e0727f 100644 (file)
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -152,5 +152,10 @@ int qemuMonitorTextAttachPCIDiskController(qemuMonitorPtr mon,
                                            const char *bus,
                                            virDomainDevicePCIAddress *guestAddr);
 
+int qemuMonitorTextAttachDrive(qemuMonitorPtr mon,
+                               const char *drivestr,
+                               virDomainDevicePCIAddress *controllerAddr,
+                               virDomainDeviceDriveAddress *driveAddr);
+
 
 #endif /* QEMU_MONITOR_TEXT_H */