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@xxxxxxxxxxx";>jakkhandy@xxxxxxxxxxx</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.)
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list