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

[Daniel P. Berrange]

On Tue, Jun 12, 2012 at 12:03:02PM -0400, Laine Stump wrote:
> 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 :-)

Yes, I tend to agree with your suggestions that we should model the
PCI addresses fully and not have their encoded in a string.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|