<div class="gmail_quote">On Thu, Mar 1, 2012 at 1:17 PM, Laine Stump <<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div>On 03/01/2012 01:19 PM, Laine Stump wrote: > On 02/29/2012 07:26 PM, Ansis Atteka wrote: >> >> >> On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a> </div><div>>> <mailto:<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>>> wrote: >> >> On 02/17/2012 02:51 PM, Ansis Atteka wrote: >> > >> > >> > On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a> >> <mailto:<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>> </div><div><div>>> > <mailto:<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a> <mailto:<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>>>> 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: > > <a href="https://www.redhat.com/archives/libvir-list/2012-March/msg00043.html" target="_blank">https://www.redhat.com/archives/libvir-list/2012-March/msg00043.html</a> > > (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. </div></div>Ansis - I posted my changes as a separate patch rather than squashed into yours, and posted it. If you're okay with my patch, I'll ACK yours and push them both together. <a href="https://www.redhat.com/archives/libvir-list/2012-March/msg00056.html" target="_blank">https://www.redhat.com/archives/libvir-list/2012-March/msg00056.html</a></blockquote><div>I am ok with this approach as well.</div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> -- libvir-list mailing list <a href="mailto:libvir-list@redhat.com" target="_blank">libvir-list@redhat.com</a> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a> </div></div></blockquote></div>