<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">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 class="im">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">laine@laine.org</a><br>
</div><div class="im">> <mailto:<a href="mailto:laine@laine.org">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 class="im">> 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 class="im">> 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 class="im">> 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>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><br></div><div>But this would not solve the issue for Domain UUID which we also</div><div>would like to set from virNetDevOpenvswitchAddPort() function.</div>
<div>So what about adding pointer in virDomainNetDefPtr structure</div><div>to virDomainDefPtr structure?</div><div><br></div><div>If you guys are ok with what I am proposing here then I could send</div><div>my updated patches for review (one for attached-mac and another</div>
<div>for vm-uuid).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Let's let this sit over the weekend and see if anyone else has a<br>
brilliant idea/insight.<br>
<div><div class="h5"><br>
><br>
> Is this a precursor to something else? Does openvswitch maybe not need<br>
> this extremely high MAC address?<br>
><br>
><br>
><br>
><br>
> > ---<br>
> > src/network/bridge_driver.c | 2 +-<br>
> > src/qemu/qemu_command.c | 5 +----<br>
> > src/uml/uml_conf.c | 5 +----<br>
> > src/util/virnetdevtap.c | 11 ++++++++++-<br>
> > src/util/virnetdevtap.h | 1 +<br>
> > 5 files changed, 14 insertions(+), 10 deletions(-)<br>
> ><br>
> > diff --git a/src/network/bridge_driver.c<br>
> b/src/network/bridge_driver.c<br>
> > index 8575d3e..3e1e031 100644<br>
> > --- a/src/network/bridge_driver.c<br>
> > +++ b/src/network/bridge_driver.c<br>
> > @@ -1766,7 +1766,7 @@ networkStartNetworkVirtual(struct<br>
> network_driver *driver,<br>
> > }<br>
> > if (virNetDevTapCreateInBridgePort(network->def->bridge,<br>
> > &macTapIfName,<br>
> network->def->mac,<br>
> > - 0, false, NULL,<br>
> NULL) < 0) {<br>
> > + false, 0, false,<br>
> NULL, NULL) < 0) {<br>
> > VIR_FREE(macTapIfName);<br>
> > goto err0;<br>
> > }<br>
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c<br>
> > index 5a34504..671054c 100644<br>
> > --- a/src/qemu/qemu_command.c<br>
> > +++ b/src/qemu/qemu_command.c<br>
> > @@ -180,7 +180,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,<br>
> > int tapfd = -1;<br>
> > int vnet_hdr = 0;<br>
> > bool template_ifname = false;<br>
> > - unsigned char tapmac[VIR_MAC_BUFLEN];<br>
> > int actualType = virDomainNetGetActualType(net);<br>
> ><br>
> > if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {<br>
> > @@ -244,9 +243,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,<br>
> > net->model && STREQ(net->model, "virtio"))<br>
> > vnet_hdr = 1;<br>
> ><br>
> > - memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);<br>
> > - tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev<br>
> MAC */<br>
> > - err = virNetDevTapCreateInBridgePort(brname, &net->ifname,<br>
> tapmac,<br>
> > + err = virNetDevTapCreateInBridgePort(brname, &net->ifname,<br>
> net->mac, true,<br>
> > vnet_hdr, true, &tapfd,<br>
> ><br>
> virDomainNetGetActualVirtPortProfile(net));<br>
> > virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);<br>
> > diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c<br>
> > index dbbbfda..c7b29a0 100644<br>
> > --- a/src/uml/uml_conf.c<br>
> > +++ b/src/uml/uml_conf.c<br>
> > @@ -127,7 +127,6 @@ umlConnectTapDevice(virConnectPtr conn,<br>
> > const char *bridge)<br>
> > {<br>
> > bool template_ifname = false;<br>
> > - unsigned char tapmac[VIR_MAC_BUFLEN];<br>
> ><br>
> > if (!net->ifname ||<br>
> > STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||<br>
> > @@ -139,9 +138,7 @@ umlConnectTapDevice(virConnectPtr conn,<br>
> > template_ifname = true;<br>
> > }<br>
> ><br>
> > - memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);<br>
> > - tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev<br>
> MAC */<br>
> > - if (virNetDevTapCreateInBridgePort(bridge, &net->ifname,<br>
> tapmac,<br>
> > + if (virNetDevTapCreateInBridgePort(bridge, &net->ifname,<br>
> net->mac, true,<br>
> > 0, true, NULL,<br>
> ><br>
> virDomainNetGetActualVirtPortProfile(net)) < 0) {<br>
> > if (template_ifname)<br>
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c<br>
> > index 0fce08d..868ba57 100644<br>
> > --- a/src/util/virnetdevtap.c<br>
> > +++ b/src/util/virnetdevtap.c<br>
> > @@ -22,6 +22,7 @@<br>
> ><br>
> > #include <config.h><br>
> ><br>
> > +#include "virmacaddr.h"<br>
> > #include "virnetdevtap.h"<br>
> > #include "virnetdev.h"<br>
> > #include "virnetdevbridge.h"<br>
> > @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname<br>
> ATTRIBUTE_UNUSED)<br>
> > * @brname: the bridge name<br>
> > * @ifname: the interface name (or name template)<br>
> > * @macaddr: desired MAC address (VIR_MAC_BUFLEN long)<br>
> > + * @discourage: whether bridge should be discouraged from using<br>
> macaddr<br>
> > * @vnet_hdr: whether to try enabling IFF_VNET_HDR<br>
> > * @tapfd: file descriptor return value for the new tap device<br>
> > * @virtPortProfile: bridge/port specific configuration<br>
> > @@ -265,11 +267,14 @@ int virNetDevTapDelete(const char *ifname<br>
> ATTRIBUTE_UNUSED)<br>
> > int virNetDevTapCreateInBridgePort(const char *brname,<br>
> > char **ifname,<br>
> > const unsigned char *macaddr,<br>
> > + bool discourage,<br>
> > int vnet_hdr,<br>
> > bool up,<br>
> > int *tapfd,<br>
> > virNetDevVPortProfilePtr<br>
> virtPortProfile)<br>
> > {<br>
> > + unsigned char tapmac[VIR_MAC_BUFLEN];<br>
> > +<br>
> > if (virNetDevTapCreate(ifname, vnet_hdr, tapfd) < 0)<br>
> > return -1;<br>
> ><br>
> > @@ -279,7 +284,11 @@ int virNetDevTapCreateInBridgePort(const<br>
> char *brname,<br>
> > * seeing the kernel allocate random MAC for the TAP<br>
> > * device before we set our static MAC.<br>
> > */<br>
> > - if (virNetDevSetMAC(*ifname, macaddr) < 0)<br>
> > + memcpy(tapmac, macaddr, VIR_MAC_BUFLEN);<br>
> > + if (discourage)<br>
> > + tapmac[0] = 0xFE; /* Discourage bridge from using TAP<br>
> dev MAC */<br>
> > +<br>
> > + if (virNetDevSetMAC(*ifname, tapmac) < 0)<br>
> > goto error;<br>
> ><br>
> > /* We need to set the interface MTU before adding it<br>
> > diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h<br>
> > index 918f3dc..fc50e22 100644<br>
> > --- a/src/util/virnetdevtap.h<br>
> > +++ b/src/util/virnetdevtap.h<br>
> > @@ -37,6 +37,7 @@ int virNetDevTapDelete(const char *ifname)<br>
> > int virNetDevTapCreateInBridgePort(const char *brname,<br>
> > char **ifname,<br>
> > const unsigned char *macaddr,<br>
> > + bool discourage,<br>
> > int vnet_hdr,<br>
> > bool up,<br>
> > int *tapfd,<br>
><br>
> --<br>
> libvir-list mailing list<br>
</div></div>> <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a> <mailto:<a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a>><br>
> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a><br>
<div class="HOEnZb"><div class="h5">><br>
><br>
<br>
--<br>
libvir-list mailing list<br>
<a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/libvir-list" target="_blank">https://www.redhat.com/mailman/listinfo/libvir-list</a><br>
</div></div></blockquote></div><br>