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 the patch
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list