>> > <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.