On Thu, Mar 1, 2012 at 10:19 AM, Laine Stump <la...@laine.org> wrote:
> On 02/29/2012 07:26 PM, Ansis Atteka wrote: > > > > On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <la...@laine.org> wrote: > >> On 02/17/2012 02:51 PM, Ansis Atteka wrote: >> > >> > >> > On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <la...@laine.org >> > <mailto:la...@laine.org>> wrote: >> > >> > On 02/16/2012 06:49 PM, Ansis Atteka wrote: >> > > Currently libvirt sets the attached-mac to altered MAC address >> > that has >> > > first byte set to FE. This patch will change that behavior by >> > using the >> > > original (unaltered) MAC address from the domain XML >> > configuration file. >> > >> > Maybe I didn't read thoroughly enough, but I don't see where it >> > changes >> > the behavior - in the cases where previously the first byte was set >> to >> > 0xFE, now you send discourage=true, and in the cases where it >> didn't, >> > now you send discourage=false. >> > >> > "discourage" means whether bridge should be discouraged to use the >> > newly added >> > TAP device's MAC address. Libvirt does that by setting the first MAC >> > address byte >> > high enough. >> > >> > And here is how this patch works: >> > >> > 1. Now in virNetDevTapCreateInBridgePort() function we always pass >> > exactly the same MAC address that was defined in XML. >> > 2. If "discourage" flag was set to true, then we create a copy of MAC >> > address and set its first byte to 0xFE >> > 3. virNetDevSetMAC() function would use the MAC address that was >> > product of #2 >> > 4. while virNetDevOpenvswitchAddPort() function would use the >> > original MAC address that was passed in #1 (this code did not need >> > to be changed so most likely that was the reason why you did not >> > notice behavior changes) >> > >> >> >> Right. That's what I missed - all I saw was every occurrence of creating >> a temporary mac address with 0xFE in the first byte replaced with adding >> "discourage=true" to the args. I didn't notice that >> virNetDevOpenvswitchAddPort() takes the macaddr (while >> virNetDevBridgeAddPort() doesn't). >> >> But that means that the tap device has been created with an >> 0xFE-initiated MAC address, and then you attach to the bridge using the >> unmodified address. Is the issue that the mac address used during the >> attach needs to match the MAC address that will be in the traffic? Do >> connections to an openvswitch bridge have an implied MAC filter on them, >> such that only that MAC address gets through? >> >> (Also, the only time discourage is false is for libvirt's virtual >> network bridges. I'm wondering if they could also use the modified MAC >> address for the tap devices - if that was the case we could just always >> create the temporary MAC address in virNetDevTapCreateInBridgePort() >> (and always set the tap device's mac to that).) >> > > We could get rid of the "discourage" argument if we would pass > virDomainNetDefPtr instead of virNetDevVPortProfilePtr structure to > virNetDevOpenvswitchAddPort() function. This approach would > also eliminate the need to pass MAC address at all to the > virNetDevOpenvswitchAddPort() function making both > APIs for Linux Bridge and OVS bridge more simpler and > similar (and this could eventually lead to abstracted bridge API). > > > Unfortunately this isn't an option. Files in the util directory can't > reference anything in the conf directory (or anywhere else). See the > followon to this patch I just posted: > I realized the same thing wen I was implementing the new patch. Is there something that prohibits us from moving util/virnetdevopenvswitch.[ch], util/virnetdevbridge.[ch] and virNetDevTapCreateInBridgePort() function from /src/utils to e.g. /src/network? It seems that they are becoming more enhanced and need to include "domain_conf.h". Otherwise, if this is not an option, then I guess we will have to pass all these values through function arguments. > > https://www.redhat.com/archives/libvir-list/2012-March/msg00043.html > > (I actually found this extra #include when doing a grep of #includes in > the conf directory to make sure I was correctly remembering this > restriction) > > I've actually been thinking about this in the back of my mind ever since > your original patch. I think the solution for the "discourage" bool may be > to replace the existing "bool up" parameter of > virNetDevTapCreateInBridgePort with a "flags" parameter, then add the > following two flags: > > typedef enum { > /* bring the interface up */ > VIR_NETDEV_TAP_CREATE_IFUP = 1 << 0, > /* Set this interface's MAC as the bridge's MAC address */ > VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 1, > } virNetDevTapCreateFlags; > > In the general case of virNetDevTapCreateInBridgePort, flags would be > (VIR_NETDEV_TAP_CREATE_IFUP), but > in the one "odd" case (where we are creating the tap device just so that > the bridge would have the provided MAC address, flags would be > (VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) (since the dummy tap device > created for this purpose doesn't get ifup'ed). > > I'm going for a short walk, then will modify your original patch to do > this and post it back to the list. > > > That doesn't help you with the uuid problem (which again can't be solved > in the way you're describing because nothing from the conf directory can be > used in the util directory). For that case, I think the least controversial > way would be adding it to the arglist all the way down the call chain. I am > curious, though, if anyone else has an opinion on the idea of putting a > "hidden" value into virNetDevVPortProfile - this would just be a sub-struct > at the end called "hidden" that would never be used by the parse or format > function, but could be used to carry around things like a copy of the > domain's uuid to all the places that use a virtPortProfile; seems like it > might be generally useful. >
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list