<br><br><div class="gmail_quote">On Thu, Mar 1, 2012 at 10:19 AM, Laine Stump <span dir="ltr"><<a href="mailto:laine@laine.org">laine@laine.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
On 02/29/2012 07:26 PM, Ansis Atteka wrote:
<blockquote type="cite"><br>
<br>
<div class="gmail_quote">On Sat, Feb 18, 2012 at 7:07 PM, Laine
Stump <span dir="ltr"><<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On 02/17/2012 02:51 PM, Ansis Atteka wrote:<br>
><br>
><br>
> On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a><br>
</div>
<div>> <mailto:<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>>>
wrote:<br>
><br>
> On 02/16/2012 06:49 PM, Ansis Atteka wrote:<br>
> > Currently libvirt sets the attached-mac to
altered MAC address<br>
> that has<br>
> > first byte set to FE. This patch will change
that behavior by<br>
> using the<br>
> > original (unaltered) MAC address from the
domain XML<br>
> configuration file.<br>
><br>
> Maybe I didn't read thoroughly enough, but I don't
see where it<br>
> changes<br>
> the behavior - in the cases where previously the
first byte was set to<br>
> 0xFE, now you send discourage=true, and in the
cases where it didn't,<br>
> now you send discourage=false.<br>
><br>
> "discourage" means whether bridge should be discouraged
to use the<br>
> newly added<br>
> TAP device's MAC address. Libvirt does that by setting
the first MAC<br>
> address byte<br>
> high enough.<br>
><br>
> And here is how this patch works:<br>
><br>
</div>
> 1. Now in virNetDevTapCreateInBridgePort() function we
always pass<br>
<div>> exactly the same MAC address that was
defined in XML.<br>
</div>
> 2. If "discourage" flag was set to true, then we create
a copy of MAC<br>
<div>> address and set its first byte to
0xFE<br>
</div>
> 3. virNetDevSetMAC() function would use the MAC address
that was<br>
> product of #2<br>
> 4. while virNetDevOpenvswitchAddPort() function would
use the<br>
<div>> original MAC address that was passed
in #1 (this code did not need<br>
> to be changed so most likely that was the reason
why you did not<br>
> notice behavior changes)<br>
><br>
<br>
<br>
</div>
Right. That's what I missed - all I saw was every occurrence
of creating<br>
a temporary mac address with 0xFE in the first byte replaced
with adding<br>
"discourage=true" to the args. I didn't notice that<br>
virNetDevOpenvswitchAddPort() takes the macaddr (while<br>
virNetDevBridgeAddPort() doesn't).<br>
<br>
But that means that the tap device has been created with an<br>
0xFE-initiated MAC address, and then you attach to the bridge
using the<br>
unmodified address. Is the issue that the mac address used
during the<br>
attach needs to match the MAC address that will be in the
traffic? Do<br>
connections to an openvswitch bridge have an implied MAC
filter on them,<br>
such that only that MAC address gets through?<br>
<br>
(Also, the only time discourage is false is for libvirt's
virtual<br>
network bridges. I'm wondering if they could also use the
modified MAC<br>
address for the tap devices - if that was the case we could
just always<br>
create the temporary MAC address in
virNetDevTapCreateInBridgePort()<br>
(and always set the tap device's mac to that).)<br>
</blockquote>
</div>
</blockquote>
<br>
<blockquote type="cite">
<div class="gmail_quote">
<div>We could get rid of the "discourage" argument if we would
pass</div>
<div>virDomainNetDefPtr instead of virNetDevVPortProfilePtr
structure to</div>
<div>virNetDevOpenvswitchAddPort() function. This approach would</div>
<div>also eliminate the need to pass MAC address at all to the</div>
<div>virNetDevOpenvswitchAddPort() function making both</div>
<div>APIs for Linux Bridge and OVS bridge more simpler and</div>
<div>similar (and this could eventually lead to abstracted
bridge API).</div>
</div>
</blockquote>
<br></div></div>
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:</div></blockquote><div>I realized the same thing wen I was implementing the new patch.</div><div><br></div><div>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".</div>
<div><br></div><div>Otherwise, if this is not an option, then I guess we will have to pass all these values through function arguments.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><br>
<a href="https://www.redhat.com/archives/libvir-list/2012-March/msg00043.html" target="_blank">https://www.redhat.com/archives/libvir-list/2012-March/msg00043.html</a><br>
<br>
(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)<br>
<br>
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:<br>
<br>
typedef enum {<br>
/* bring the interface up */<br>
VIR_NETDEV_TAP_CREATE_IFUP = 1 << 0,<br>
/* Set this interface's MAC as the bridge's MAC address */<br>
VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 1,<br>
} virNetDevTapCreateFlags;<br>
<br>
In the general case of virNetDevTapCreateInBridgePort, flags would
be (VIR_NETDEV_TAP_CREATE_IFUP), but<br>
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).<br>
<br>
I'm going for a short walk, then will modify your original patch to
do this and post it back to the list.<br>
<br>
<br>
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.</div></blockquote></div>