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 :-)