[Qemu-devel] [PATCH 7/9] Remove surprises in the semantics of disk-hotadd

[Wolfgang Mauerer]

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 <address@hidden>
Signed-off-by: Jan Kiszka <address@hidden>
---
 src/qemu_driver.c |  172 ++++++++++++++++++++++++++---------------------------
 1 files changed, 85 insertions(+), 87 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 990f05a..f1c2a45 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -5417,68 +5417,81 @@ try_command:
         controller_specified = 1;
     }
 
-    if (controller_specified) {
-        if (dev->data.disk->controller_id) {
-            for (i = 0 ; i < vm->def->ncontrollers ; i++) {
-                if (STREQ(dev->data.disk->controller_id, 
-                          vm->def->controllers[i]->id)) {
-                    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]->pci_addr.domain;
-            bus    = vm->def->controllers[i]->pci_addr.bus;
-            slot   = vm->def->controllers[i]->pci_addr.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]->pci_addr.domain &&
-                    bus    ==  vm->def->controllers[i]->pci_addr.bus &&
-                    slot   ==  vm->def->controllers[i]->pci_addr.slot)
-                    break;
-            }
+        domain = vm->def->controllers[i]->pci_addr.domain;
+        bus    = vm->def->controllers[i]->pci_addr.bus;
+        slot   = vm->def->controllers[i]->pci_addr.slot;
+    }
 
-            if (i == vm->def->ncontrollers) {
-                qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                                 _("Controller does not exist"));
-                return -1;
+    if (controller_specified && dev->data.disk->controller_id) {
+        for (i = 0 ; i < vm->def->ncontrollers ; i++) {
+            if (STREQ(dev->data.disk->controller_id, 
+                      vm->def->controllers[i]->id)) {
+                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]->pci_addr.domain;
+        bus    = vm->def->controllers[i]->pci_addr.bus;
+        slot   = vm->def->controllers[i]->pci_addr.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]->pci_addr.domain &&
+                bus    ==  vm->def->controllers[i]->pci_addr.bus &&
+                slot   ==  vm->def->controllers[i]->pci_addr.slot)
+                break;
         }
+        
+        if (i == vm->def->ncontrollers) {
+            qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                             _("Controller does not exist"));
+            return -1;
+        }
+    }
 
-        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);
+    /* 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);
+    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);
+    
     VIR_FREE(safe_path);
     if (ret == -1) {
         virReportOOMError(conn);
@@ -5494,41 +5507,26 @@ try_command:
 
     VIR_FREE(cmd);
 
-    if (controller_specified) {
-        if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) {
-           if (!tryOldSyntax && strstr(reply, "invalid char in expression")) {
-               VIR_FREE(reply);
-               tryOldSyntax = 1;
-               goto try_command;
-           }
-           qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                             _("adding %s disk failed: %s"), type, reply);
-           VIR_FREE(reply);
-           return -1;
-       }
-
-       if (dev->data.disk->bus_id == -1) {
-           dev->data.disk->bus_id = bus_id;
-       }
-
-       if (dev->data.disk->unit_id == -1) {
-           dev->data.disk->unit_id = unit_id;
-       }
-    } else {
-        if (qemudParsePciAddReply(vm, reply, &domain, &bus, &slot) < 0) {
-           if (!tryOldSyntax && strstr(reply, "invalid char in expression")) {
-               VIR_FREE(reply);
-               tryOldSyntax = 1;
-               goto try_command;
-           }
-           
-           qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                             _("adding %s disk failed: %s"), type, reply);
-           VIR_FREE(reply);
-           return -1;
-       }
+    if (qemudParseDriveAddReply(vm, reply, &bus_id, &unit_id) < 0) {
+        if (!tryOldSyntax && strstr(reply, "invalid char in expression")) {
+            VIR_FREE(reply);
+            tryOldSyntax = 1;
+            goto try_command;
+        }
+        qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                          _("adding %s disk failed: %s"), type, reply);
+        VIR_FREE(reply);
+        return -1;
     }
-       
+    
+    if (dev->data.disk->bus_id == -1) {
+        dev->data.disk->bus_id = bus_id;
+    }
+    
+    if (dev->data.disk->unit_id == -1) {
+        dev->data.disk->unit_id = unit_id;
+    }
+    
     dev->data.disk->pci_addr.domain = domain;
     dev->data.disk->pci_addr.bus    = bus;
     dev->data.disk->pci_addr.slot   = slot;
-- 
1.6.4