[libvirt] [PATCH 10/13] Factor out the method to get the PCI address of a controller for a given disk

Monday, 16 November 2009

From: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt;

We need this multiple times later on.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer(a)siemens.com&gt;
Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com&gt;
---
 src/conf/domain_conf.c |   26 ++----
 src/conf/domain_conf.h |    8 ++
 src/qemu/qemu_driver.c |  242 +++++++++++++++++++++++++++---------------------
 3 files changed, 153 insertions(+), 123 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e90975f..48015a2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1077,37 +1077,20 @@ virDomainDiskDefParseXML(virConnectPtr conn,
 	def->unit_id = -1;
 	if (unit_id && virStrToLong_i(unit_id, NULL, 10, &def->unit_id) < 0)
{
 	    virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
-			 _("Cannot parse <controller> 'unit' attribute"));
-	    goto error;
-	}
-
-	/* TODO: Should we re-use devaddr for this purpose,
-           or is an extra field justified? */
-	if (controller_pci_addr &&
-	    sscanf(controller_pci_addr, "%x:%x:%x",
-		   &def->controller_pci_addr.domain,
-		   &def->controller_pci_addr.bus,
-		   &def->controller_pci_addr.slot) < 3) {
-	    virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
-		  _("Unable to parse <controller> 'pci_addr' parameter
'%s'"),
-				 controller_pci_addr);
+                               _("Cannot parse <controller> 'unit'
attribute"));
 	    goto error;
 	}
     }
 
     /* TODO: Should we re-use devaddr for this purpose,
        or is an extra field justified? */
-    def->controller_pci_addr.domain = -1;
-    def->controller_pci_addr.bus = -1;
-    def->controller_pci_addr.slot = -1;
-
     if (controller_pci_addr &&
         sscanf(controller_pci_addr, "%x:%x:%x",
                &def->controller_pci_addr.domain,
                &def->controller_pci_addr.bus,
                &def->controller_pci_addr.slot) < 3) {
         virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
-                             _("Unable to parse <controller> 'pciaddr'
parameter '%s'"),
+                   _("Unable to parse <controller> 'pci_addr'
parameter '%s'"),
                              controller_pci_addr);
         goto error;
     }
@@ -1215,6 +1198,11 @@ virDomainControllerDefParseXML(virConnectPtr conn,
     }
 
     def->name = virXMLPropString(node, "name");
+    if (!def->name) {
+        virDomainReportError(conn, VIR_ERR_INVALID_ARG,
+                             _("cannot use controller without name"));
+        goto error;
+    }
 
     cur = node->children;
     while (cur != NULL) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b30d08c..e058c56 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -171,6 +171,14 @@ virDiskHasValidPciAddr(virDomainDiskDefPtr def)
     return def->pci_addr.domain || def->pci_addr.bus || def->pci_addr.slot;
 }
 
+static inline int
+virDiskHasValidController(virDomainDiskDefPtr def)
+{
+    return def->controller_name != NULL ||
+        def->controller_pci_addr.domain || def->controller_pci_addr.domain
+        || def->controller_pci_addr.slot;
+}
+
 
 /* Two types of disk backends */
 enum virDomainFSType {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4f647c4..9f44685 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4356,44 +4356,20 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
     return ret;
 }
 
-static int qemudDomainAttachDiskDevice(virConnectPtr conn,
-                                       virDomainObjPtr vm,
-                                       virDomainDeviceDefPtr dev)
-{
-    int ret, i;
-    char *cmd, *reply;
-    char *safe_path;
-    /* NOTE: Later patch will use type of the controller instead
-       of the disk */
-    const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus);
-    int tryOldSyntax = 0;
-    unsigned domain, bus, slot;
-    int bus_id, unit_id;
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
-    char* bus_unit_string;
-    int controller_specified;
-
-    for (i = 0 ; i < vm->def->ndisks ; i++) {
-        if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
-            qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                           _("target %s already exists"),
dev->data.disk->dst);
-            return -1;
-        }
-    }
-
-    if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
-        virReportOOMError(conn);
-        return -1;
-    }
-
-try_command:
-    safe_path = qemudEscapeMonitorArg(dev->data.disk->src);
-    if (!safe_path) {
-        virReportOOMError(conn);
-        return -1;
-    }
+enum {
+    CONTROLLER_FOUND      =  0,
+    NO_CONTROLLER_FOUND   = -1, /* None specified, and none found */
+    CONTROLLER_INEXISTENT = -2, /* Controller specified, but not found */
+};
+static virDomainControllerDefPtr
+qemudGetDiskController(virDomainObjPtr vm, virDomainDeviceDefPtr dev,
+                       int* ret) {
+    int controller_specified = 0;
+    int domain, bus, slot;
+    virDomainControllerDefPtr conPtr = NULL;
+    int i;
+    *ret = CONTROLLER_FOUND;
 
-    controller_specified = 0;
     /* Partially specified PCI addresses (should not happen, anyway)
        don't count as specification */
     if (dev->data.disk->controller_name != NULL ||
@@ -4411,16 +4387,17 @@ try_command:
           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)
+           if (vm->def->controllers[i]->type == dev->data.disk->type) {
+               conPtr = vm->def->controllers[i];
                break;
+           }
        }
 
-       if (i == vm->def->ncontrollers) {
-           qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                         _("Cannot find appropriate controller for hot-add"));
-           return -1;
+       if (!conPtr) {
+           *ret = NO_CONTROLLER_FOUND;
        }
 
+
        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;
@@ -4428,21 +4405,20 @@ try_command:
 
    if (controller_specified && dev->data.disk->controller_name) {
        for (i = 0 ; i < vm->def->ncontrollers ; i++) {
-           if (STREQ(dev->data.disk->controller_name,
+           if (vm->def->controllers[i]->name &&
+               STREQ(dev->data.disk->controller_name,
                      vm->def->controllers[i]->name)) {
+               conPtr = vm->def->controllers[i];
+               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;
-       }
-
-       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 (!conPtr) {
+            *ret = CONTROLLER_INEXISTENT;
+        }
    } else if (controller_specified) {
        domain = dev->data.disk->controller_pci_addr.domain;
        bus    = dev->data.disk->controller_pci_addr.bus;
@@ -4452,35 +4428,109 @@ try_command:
            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)
+               conPtr = vm->def->controllers[i];
                break;
         }
 
-       if (i == vm->def->ncontrollers) {
+        if (!conPtr) {
+            *ret = CONTROLLER_INEXISTENT;
+        }
+    }
+
+    return conPtr;
+}
+
+static int qemudDomainAttachDiskDevice(virConnectPtr conn,
+				       virDomainObjPtr vm,
+				       virDomainDeviceDefPtr dev)
+{
+    int ret, i;
+    char *cmd, *reply;
+    char *safe_path;
+    const char* type = NULL;
+    int tryOldSyntax = 0;
+    int bus_id, unit_id;
+    int domain, bus, slot;
+    virDomainControllerDefPtr conPtr;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    char* bus_unit_string;
+
+    /* Both controller and disk can specify a type like SCSI. While
+       a virtual disk as such is not bound to a specific bus type,
+       the specification is here for historical reasons. When controller
+       and disk type specification disagree, the former takes precedence
+       and we print a warning message, but still add the disk. */
+    if (virDiskHasValidController(dev->data.disk)) {
+        conPtr = qemudGetDiskController(vm, dev, &ret);
+        if (conPtr && dev->data.disk->bus != conPtr->type) {
+            VIR_WARN0(_("Controller and disk bus type disagree, controller " \
+                        "takes precedence\n"));
+            type = virDomainDiskBusTypeToString(conPtr->type);
+        }
+    }
+    if (!type) {
+        type = virDomainDiskBusTypeToString(dev->data.disk->bus);
+    }
+
+    for (i = 0 ; i < vm->def->ndisks ; i++) {
+        if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
+            qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                           _("target %s already exists"),
dev->data.disk->dst);
+            return -1;
+        }
+    }
+
+    if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) {
+        virReportOOMError(conn);
+        return -1;
+    }
+
+try_command:
+    safe_path = qemudEscapeMonitorArg(dev->data.disk->src);
+    if (!safe_path) {
+        virReportOOMError(conn);
+        return -1;
+    }
+
+    conPtr = qemudGetDiskController(vm, dev, &ret);
+    if (!conPtr) {
+        switch (ret) {
+        case CONTROLLER_INEXISTENT:
            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) {
+           break;  /* A bit pointless */
+        case NO_CONTROLLER_FOUND:
+            qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                             _("Cannot find appropriate controller for
hot-add"));
+            return -1;
+            break;
+        }
+    }
+
+    /* At this point, we have a valid controller, regardless if an explicit
+       controller has been specified or not. */
+    domain = conPtr->address->data.pci.domain;
+    bus    = conPtr->address->data.pci.bus;
+    slot   = conPtr->address->data.pci.slot;
+
+    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 (dev->data.disk->unit_id != -1) {
+        virBufferVSprintf(&buf, ",unit=%d",
dev->data.disk->unit_id);
     }
 
-   bus_unit_string = virBufferContentAndReset(&buf);
+    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_DEBUG ("%s: drive_add %.2x:%.2x:%.2x 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%.2x:%.2x:%.2x 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) {
@@ -4497,39 +4547,24 @@ 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 (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->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 (dev->data.disk->unit_id == -1) {
+        dev->data.disk->unit_id = unit_id;
     }
 
     dev->data.disk->pci_addr.domain = domain;
@@ -4538,7 +4573,6 @@ try_command:
 
     virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
 
-
     return 0;
 }
 
@@ -4590,7 +4624,7 @@ try_command:
         bus    = dev->data.controller->address->data.pci.bus;
         slot   = dev->data.controller->address->data.pci.slot;
 
-	ret = virAsprintf(&tmp, "%d:%d:%d", domain, bus, slot);
+	ret = virAsprintf(&tmp, "%.2x:%.2x:%.2x", domain, bus, slot);
         ret |= virAsprintf(&cmd, "pci_add %s%s storage if=%s",
 			   (tryOldSyntax ? "" : "pci_addr="), tmp, type);
     } else {
-- 
1.6.4


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH 10/13] Factor out the method to get the PCI address of a controller for a given disk