12:19 p.m.
On 02/29/2012 07:26 PM, Ansis Atteka wrote:
On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <laine(a)laine.org
<mailto:laine@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(a)laine.org
<mailto:laine@laine.org>
> <mailto:laine@laine.org <mailto:laine@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.
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.