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