On 02/14/2012 09:26 PM, Laine Stump wrote: > On 02/14/2012 05:33 PM, Eric Blake wrote: >> On 02/13/2012 10:56 PM, Laine Stump wrote: >> >>> But because an interfaceid is auto-generated by the parser when one >>> isn't given, and a common parser function for virtualport is used by the >>> domain and network parsers, this will result in an interfaceid being >>> filled in when the network is defined, which makes no sense for a >>> <network> definition, since each interface that uses this network is >>> supposed to have its own unique interfaceid. However, the code decides >>> that the bridge is an openvswitch by looking for the presence of a >>> virtualport of type='openvswitch'. >>> >>> I can see a couple ways out of this: >>> >>> 1) we could add an arg to virNetDevVPortProfileParse: >>> >>> virNetDevVPortProfilePtr >>> virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags); >>> >>> with a possible flag being VIR_NETDEV_VPORT_AUTOGEN >>> >>> If that flag is given, interfaceid and instanceid will be auto-generated >>> if necessary during parse, otherwise they will be left blank. >> That actually sounds reasonable to me. We've been bit more than once by >> autogenerating stuff too eagerly (most recently, Osier's patches for WWN >> auto-generation had the issue that part of the WWN is the OUI which >> should be hypervisor-specific, but the parser is supposed to be >> hypervisor-agnostic; other examples are that detach-device of a partial >> XML <interface> snippet without a MAC address would autogenerate a new >> MAC address, then fail to detach the actual intended interface). >> >>> 2) Rather than deciding which type of bridge we're dealing with by >>> looking at the config, we could try to do it by direct examination of >>> the system. >> ... >>> So, I'm wondering if there's a simple way to confirm that a device >>> *isn't* an openvswitch without relying on calling any utilities that are >>> part of the ovs package. >> I'm not sure how easy or hard this would be. At any rate, >> >>> I'm thinking it may be okay to push this code more or less as-is though, >>> since it works properly for the case of straight <interface >>> type='bridge'/>, and the changes I'm talking about are refinements that >>> are backward compatible, and are only necessary when we add support for >>> <network>s that use openvswitch. Any other libvirt regulars have an >>> opinion on this? >> in answer to this question, I think that you are correct - anything we >> do to further expand the code (whether option 1 of adding a flag to >> control whether auto-generation happens, or option 2 of probing the >> system device, or some other option 3 or even combination) can deal with >> the added complexity at that time. >> >>>> @@ -13832,15 +13854,27 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr iface) >>>> } >>>> >>>> virNetDevVPortProfilePtr >>>> -virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface) >>>> +virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) >>> In hindsight, renaming this function would have been better done in a >>> separate pre-requisite patch, just to avoid clutter in this patch. >> Yes, I highly agree that separating renames, code motion, and actual new >> code into separate patches almost always makes life easier, both in the >> initial reviews, and in later efforts to backport patches to older >> stable releases. >> >>> I didn't see any major problems in this version, so I'm inclined to ACK >>> it conditioned on the nits being fixed (they're simple enough I could >>> just take care of them while pushing it). The one thing I would like to >>> get other opinions of before pushing is problem with auto-generating the >>> interfaceid (just in case there's a better way to fix it that would >>> required making a change now). >> I think we're safe pushing now. > I'll fix up the few nits and push it either later tonight or in the > morning, then :-) Okay, I fixed all the whitespace nits, put the function rename into a separate patch, added virnetdevopenvswitch.c to po/POTFILES.in, and added all three contributors to AUTHORS, then pushed the result (after successful make check && make syntax-check). Welcome to libvirt, openvswitch! (and thanks for the contribution, Ansis, Dan, and Kyle!) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list