Re: [libvirt] [PATCH 1/5] Code to return interface name or pci_addr of the VF in actualDevice
On 06/11/2012 01:14 PM, Laine Stump wrote:
On 06/11/2012 11:50 AM, Daniel P. Berrange wrote:
> On Fri, Jun 08, 2012 at 04:29:14PM +0100, Shradha Shah wrote:
>> @@ -925,6 +931,44 @@ error:
>> return result;
>> }
>>
>> +/* Function to compare strings with wildcard strings*/
>> +/* When editing this function also edit the one in
src/network/bridge_driver.c*/
>> +static int
>> +wildcmp(const char *wild, const char *string)
>> +{
>> +/* Written by Jack Handy - <A
href="mailto:jakkhandy@hotmail.com">jakkhandy@hotmail.com</A>*/
> Isn't this just reimplementing the Glibc 'fnmatch' function that
> we already use ?
Beyond that, this is being used to support another case of crowding
multiple data items into a single attribute, which we actively
discourage (see the current thread about dhcpsnoop :-).
Instead of re-purposing the singular char *dev, it would be better to
follow the example of, e.g., virDomainDeviceInfo, and have a union like
this:
int type;
union {
virDomainDevicePCIAddress pci;
char *dev;
} addr;
or just put both directly in the struct if you might want both to be
populated at the same time for some reason (maybe you want to derive all
the PCI addresses from the netdev name, or vice versa, and cache them
here. Or maybe you don't, I don't have an opinion).
The XML for this would then look something like this:
<network>
<name>hostdev-network</name>
<forward mode="hostdev">
<interface type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x01'/>
<interface type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x02'/>
<interface type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x03'/>
</forward>
</network>
Or maybe, for consistency with source addresses in domain device
definitions, it should be:
<network>
<name>hostdev-network</name>
<forward mode="hostdev">
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x01'/>
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x02'/>
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00'
function='0x03'/>
</forward>
</network>
Also, about the "managed" attribute you introduced in PATCH 5/5 (which I
suggested you should add at the same time as the rest of the new XML):
Do you really think anyone will ever want some of the interfaces to be
managed='yes' and some managed='no'? If you rework the XML as I suggest
above, you would be able to reuse the same function to parse and format
pci addresses; if you add a "managed" attribute to each address element,
you would no longer be able to do that.
Alternately, if you just add a single managed attribute to <forward>,
the <address> sub-element would remain identical to the <address> in the
domain devices' <source> element, so the parse/format functions could be
shared (and the consistency would also make mixups less likely).
Something like this:
<network>
<name>hostdev-network</name>
<forward mode="hostdev" managed="yes">
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x01'/>
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x02'/>
<address type='pci' domain='0x0000' bus='0x04'
slot='0x00' function='0x03'/>
</forward>
</network
You lose some flexibility, but are able to re-use more code. If it never
makes sense to mix managed and un-managed (I don't know enough about the
topic to have the answer to that), then the lost flexibility is meaningless.
(anyone else have an opinion on this?)
The same question holds for this new addition :-)
As a preparation for this, you should probably have a prerequisite
patch
that renames virDomainDevicePCIAddress to virDevicePCIAddress, moves it
to the new file device_conf.h, and also moves
virDomainDevicePCIAddressParseXML into device_conf.c (renaming it on the
way). Similarly, the part of virDomainHostdevSourceFormat that formats a
pci address could/should be moved into its own function in device_conf.c.
(I'm suggesting the name "device_conf.c" rather than
"pci_device_conf.c"
because I'm thinking it might be good to rename/move the other
virDomainDevice*Address structs and functions to the same place. Before
taking the time to make this reorganization, it might be good to get
feedback from some other people.)