[libvirt] [PATCH 2/9] Extend <disk> element with controller information

[Wolfgang Mauerer]

Daniel P. Berrange wrote:
> 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.

I'm find with s/id/name/g.

I completely agree with you that using a name instead of
a PCI address would suit the generic spirit of libvirt much
better. However, do all hypervisors allow us to attach
individual controllers, or are there cases where
controller are implicitely added to the system? In this
case, assigning names to controllers might be impractical
or maybe impossible without further ado. So for the time
being, I would suggest to keep the pci_addr parameter,
but clearly state that name is much preferred.

> 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

> There's a few whitespace issues here - I guess some tabs crept
> into the patch

Whops - I'll eliminate them in the next series.

Thanks, Wolfgang