[libvirt] [PATCH 1/5] Code to return interface name or pci_addr of the VF in actualDevice

[Laine Stump]

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 at hotmail.com">jakkhandy at 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.)
>