[libvirt] [PATCH] Use the same MAC address that is defined in domain XML for attached-mac field.

[Laine Stump]

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