[libvirt] [PATCH] Use the same MAC address that is defined in domain XML for attached-mac field.
Ansis Atteka
aatteka at nicira.com
Fri Mar 2 01:54:13 UTC 2012
On Thu, Mar 1, 2012 at 1:17 PM, Laine Stump <laine at laine.org> wrote:
> 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 <laine at laine.org
> >> <mailto:laine at laine.org>> wrote:
> >>
> >> On 02/17/2012 02:51 PM, Ansis Atteka wrote:
> >> >
> >> >
> >> > On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <laine at laine.org
> >> <mailto:laine at laine.org>
> >> > <mailto:laine at laine.org <mailto:laine at 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:
> >
> > 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.
>
> 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.
>
> https://www.redhat.com/archives/libvir-list/2012-March/msg00056.html
I am ok with this approach as well.
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120301/f72429e1/attachment-0001.htm>
More information about the libvir-list
mailing list