[libvirt] [PATCH 09/13] Remove surprises in the semantics of disk-hotadd

[wolfgang.mauerer at siemens.com]

From: Wolfgang Mauerer <wolfgang.mauerer at siemens.com>

When a disk is added without an explicitly specified
controller as host, then try to find the first available
controller. If none exists, do not (as in previous versions)
add a new PCI controller device with the disk attached,
but bail out with an error. Notice that this patch changes
the behaviour as compared to older libvirt releases, as
has been discussed on the mailing list (see
http://thread.gmane.org/gmane.comp.emulators.libvirt/15860)

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer at siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
---
 src/qemu/qemu_driver.c |  122 +++++++++++++++++++++++++++---------------------
 1 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d3395a7..4f647c4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4406,68 +4406,82 @@ try_command:
         controller_specified = 1;
     }
 
-    if (controller_specified) {
-        if (dev->data.disk->controller_name) {
-            for (i = 0 ; i < vm->def->ncontrollers ; i++) {
-                if (STREQ(dev->data.disk->controller_name,
-                          vm->def->controllers[i]->name)) {
-                    break;
-                }
-            }
-
-            if (i == vm->def->ncontrollers) {
-                qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                                 _("Controller does not exist"));
-                return -1;
-            }
-
-            domain = vm->def->controllers[i]->address->data.pci.domain;
-            bus    = vm->def->controllers[i]->address->data.pci.bus;
-            slot   = vm->def->controllers[i]->address->data.pci.slot;
-        } else {
-            domain = dev->data.disk->controller_pci_addr.domain;
-            bus    = dev->data.disk->controller_pci_addr.bus;
-            slot   = dev->data.disk->controller_pci_addr.slot;
-
-            for (i = 0 ; i < vm->def->ncontrollers ; i++) {
-                if (domain == vm->def->controllers[i]->address->data.pci.domain &&
-                    bus    == vm->def->controllers[i]->address->data.pci.bus &&
-                    slot   == vm->def->controllers[i]->address->data.pci.slot)
-                    break;
-            }
-
-            if (i == vm->def->ncontrollers) {
-                qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                                 _("Controller does not exist"));
-                return -1;
+   if (!controller_specified) {
+       /* Find an appropriate controller for disk-hotadd. Notice that
+          the admissible controller types (SCSI, virtio) have already
+          been checked in qemudDomainAttachDevice. */
+       for (i = 0 ; i < vm->def->ncontrollers ; i++) {
+           if (vm->def->controllers[i]->type == dev->data.disk->type)
+               break;
+       }
+
+       if (i == vm->def->ncontrollers) {
+           qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                         _("Cannot find appropriate controller for hot-add"));
+           return -1;
+       }
+
+       domain = vm->def->controllers[i]->address->data.pci.domain;
+       bus    = vm->def->controllers[i]->address->data.pci.bus;
+       slot   = vm->def->controllers[i]->address->data.pci.slot;
+   }
+
+   if (controller_specified && dev->data.disk->controller_name) {
+       for (i = 0 ; i < vm->def->ncontrollers ; i++) {
+           if (STREQ(dev->data.disk->controller_name,
+                     vm->def->controllers[i]->name)) {
+               break;
             }
-	}
-
-        if (dev->data.disk->bus_id != -1) {
-            virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id);
         }
 
-        if (dev->data.disk->unit_id != -1) {
-            virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id);
+       if (i == vm->def->ncontrollers) {
+           qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                            _("Controller does not exist"));
+           return -1;
+       }
+
+       domain = vm->def->controllers[i]->address->data.pci.domain;
+       bus    = vm->def->controllers[i]->address->data.pci.bus;
+       slot   = vm->def->controllers[i]->address->data.pci.slot;
+   } else if (controller_specified) {
+       domain = dev->data.disk->controller_pci_addr.domain;
+       bus    = dev->data.disk->controller_pci_addr.bus;
+       slot   = dev->data.disk->controller_pci_addr.slot;
+
+       for (i = 0 ; i < vm->def->ncontrollers ; i++) {
+           if (domain ==  vm->def->controllers[i]->address->data.pci.domain &&
+               bus    ==  vm->def->controllers[i]->address->data.pci.bus &&
+               slot   ==  vm->def->controllers[i]->address->data.pci.slot)
+               break;
         }
 
-        bus_unit_string = virBufferContentAndReset(&buf);
-
-        VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name,
-                   domain, bus, slot, safe_path, type, bus_unit_string);
-        ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s",
-			  (tryOldSyntax ? "" : "pci_addr="), domain, bus,
-                          slot, safe_path, type, bus_unit_string);
+       if (i == vm->def->ncontrollers) {
+           qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                            _("Controller does not exist"));
+           return -1;
+       }
+   }
+   /* At this point, we have a valid (domain, bus, slot) triple
+      that identifies the controller, regardless if an explicit
+      controller has been given or not. */
+   if (dev->data.disk->bus_id != -1) {
+       virBufferVSprintf(&buf, ",bus=%d", dev->data.disk->bus_id);
     }
-    else {
-        /* Old-style behaviour: If no <controller> reference is given, then
-           hotplug a new controller with the disk attached. */
-        VIR_DEBUG ("%s: pci_add %s storage file=%s,if=%s", vm->def->name,
-                   (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type);
-        ret = virAsprintf(&cmd, "pci_add %s storage file=%s,if=%s",
-                   (tryOldSyntax ? "0": "pci_addr=auto"), safe_path, type);
+
+   if (dev->data.disk->unit_id != -1) {
+       virBufferVSprintf(&buf, ",unit=%d", dev->data.disk->unit_id);
     }
 
+   bus_unit_string = virBufferContentAndReset(&buf);
+
+   VIR_DEBUG ("%s: drive_add %d:%d:%d file=%s,if=%s%s", vm->def->name,
+              domain, bus, slot, safe_path, type,
+              bus_unit_string ? bus_unit_string : "");
+   ret = virAsprintf(&cmd, "drive_add %s%d:%d:%d file=%s,if=%s%s",
+                     (tryOldSyntax ? "" : "pci_addr="), domain, bus,
+                     slot, safe_path, type,
+                     bus_unit_string ? bus_unit_string : "");
+
     VIR_FREE(safe_path);
     if (ret == -1) {
         virReportOOMError(conn);
-- 
1.6.4