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