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

[Ansis Atteka]

On Thu, Mar 1, 2012 at 10:19 AM, Laine Stump <laine at laine.org> 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> 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>> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120301/20affce4/attachment-0001.htm>