On Fri, Sep 18, 2009 at 05:26:09PM +0200, Wolfgang Mauerer wrote: > This allows us to connect a disk with a specific controller, > which is required for disk hotadd/remove. A new XML child > element is added to the <disk> container: > > <disk> > ... > <controller pci_addr="<addr>" id="<identifier>" bus="<number>" unit="<number>"/> > </disk> > > Either id _or_ pci_addr can be specified. When the controller > has been brought into the system via tghe hotplug mechanism also > included in this patch series, it will typically have an ID. > In other cases, the controller can also be specified with > the PCI address. I think it would be desriable to make 'id' the mandatory argument and not use 'pci_addr' in this area of the XML. <controller id="<identifier>" bus="<number>" unit="<number>"/> If fact I think it'd be good to rename 'id' to 'name' since in libvirt context we typically use 'id' to be be positive integer identifier. 'name' reflects the fact that this device idenifier is really a string. Outside the scope of your patches, I think it would be worth adding a 'name' attribute to all devices in the libvirt XML as a standardized unique identifier. We already have to keep track of a 'name' internally for NIC hotplug with QEMU, and with QEMU's qdev work we're going to end up having to track a 'name' for pretty much all devices. Thus we might as well expose it in the XML > memmove(target, target+6, strlen(target)-5); > + } else if ((controller == NULL) && > + (xmlStrEqual(cur->name, BAD_CAST "controller"))) { > + controller_id = virXMLPropString(cur, "id"); > + bus_id = virXMLPropString(cur, "bus"); > + unit_id = virXMLPropString(cur, "unit"); > } else if ((driverName == NULL) && > (xmlStrEqual(cur->name, BAD_CAST "driver"))) { > driverName = virXMLPropString(cur, "name"); > @@ -800,6 +811,24 @@ virDomainDiskDefParseXML(virConnectPtr conn, > } > } > > + if (controller) { > + def->controller_id = controller_id; > + > + def->bus_id = -1; > + if (bus_id && virStrToLong_i(bus_id, NULL, 10, &def->bus_id) < 0) { > + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot parse <controller> 'bus' attribute")); > + goto error; > + } > + > + 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; > + } > + } > + There's a few whitespace issues here - I guess some tabs crept into :|