1 Mar 2012 01:26
Re: [libvirt] [PATCH] Use the same MAC address that is defined in domain XML for attached-mac field.
Ansis Atteka <aatteka <at> nicira.com>
2012-03-01 00:26:10 GMT
2012-03-01 00:26:10 GMT
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:> 1. Now in virNetDevTapCreateInBridgePort() function we always pass
>
> 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:
>
> 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 needRight. That's what I missed - all I saw was every occurrence of creating
> to be changed so most likely that was the reason why you did not
> notice behavior changes)
>
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).
But this would not solve the issue for Domain UUID which we also
would like to set from virNetDevOpenvswitchAddPort() function.
So what about adding pointer in virDomainNetDefPtr structure
to virDomainDefPtr structure?
If you guys are ok with what I am proposing here then I could send
my updated patches for review (one for attached-mac and another
for vm-uuid).
Let's let this sit over the weekend and see if anyone else has a
brilliant idea/insight.> libvir-list <at> redhat.com <mailto:libvir-list <at> redhat.com>
>
> Is this a precursor to something else? Does openvswitch maybe not need
> this extremely high MAC address?
>
>
>
>
> > ---
> > src/network/bridge_driver.c | 2 +-
> > src/qemu/qemu_command.c | 5 +----
> > src/uml/uml_conf.c | 5 +----
> > src/util/virnetdevtap.c | 11 ++++++++++-
> > src/util/virnetdevtap.h | 1 +
> > 5 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/network/bridge_driver.c
> b/src/network/bridge_driver.c
> > index 8575d3e..3e1e031 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > <at> <at> -1766,7 +1766,7 <at> <at> networkStartNetworkVirtual(struct
> network_driver *driver,
> > }
> > if (virNetDevTapCreateInBridgePort(network->def->bridge,
> > &macTapIfName,
> network->def->mac,
> > - 0, false, NULL,
> NULL) < 0) {
> > + false, 0, false,
> NULL, NULL) < 0) {
> > VIR_FREE(macTapIfName);
> > goto err0;
> > }
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 5a34504..671054c 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > <at> <at> -180,7 +180,6 <at> <at> qemuNetworkIfaceConnect(virDomainDefPtr def,
> > int tapfd = -1;
> > int vnet_hdr = 0;
> > bool template_ifname = false;
> > - unsigned char tapmac[VIR_MAC_BUFLEN];
> > int actualType = virDomainNetGetActualType(net);
> >
> > if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > <at> <at> -244,9 +243,7 <at> <at> qemuNetworkIfaceConnect(virDomainDefPtr def,
> > net->model && STREQ(net->model, "virtio"))
> > vnet_hdr = 1;
> >
> > - memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> > - tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev
> MAC */
> > - err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
> tapmac,
> > + err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
> net->mac, true,
> > vnet_hdr, true, &tapfd,
> >
> virDomainNetGetActualVirtPortProfile(net));
> > virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
> > diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> > index dbbbfda..c7b29a0 100644
> > --- a/src/uml/uml_conf.c
> > +++ b/src/uml/uml_conf.c
> > <at> <at> -127,7 +127,6 <at> <at> umlConnectTapDevice(virConnectPtr conn,
> > const char *bridge)
> > {
> > bool template_ifname = false;
> > - unsigned char tapmac[VIR_MAC_BUFLEN];
> >
> > if (!net->ifname ||
> > STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
> > <at> <at> -139,9 +138,7 <at> <at> umlConnectTapDevice(virConnectPtr conn,
> > template_ifname = true;
> > }
> >
> > - memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> > - tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev
> MAC */
> > - if (virNetDevTapCreateInBridgePort(bridge, &net->ifname,
> tapmac,
> > + if (virNetDevTapCreateInBridgePort(bridge, &net->ifname,
> net->mac, true,
> > 0, true, NULL,
> >
> virDomainNetGetActualVirtPortProfile(net)) < 0) {
> > if (template_ifname)
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index 0fce08d..868ba57 100644
> > --- a/src/util/virnetdevtap.c
> > +++ b/src/util/virnetdevtap.c
> > <at> <at> -22,6 +22,7 <at> <at>
> >
> > #include <config.h>
> >
> > +#include "virmacaddr.h"
> > #include "virnetdevtap.h"
> > #include "virnetdev.h"
> > #include "virnetdevbridge.h"
> > <at> <at> -248,6 +249,7 <at> <at> int virNetDevTapDelete(const char *ifname
> ATTRIBUTE_UNUSED)
> > * <at> brname: the bridge name
> > * <at> ifname: the interface name (or name template)
> > * <at> macaddr: desired MAC address (VIR_MAC_BUFLEN long)
> > + * <at> discourage: whether bridge should be discouraged from using
> macaddr
> > * <at> vnet_hdr: whether to try enabling IFF_VNET_HDR
> > * <at> tapfd: file descriptor return value for the new tap device
> > * <at> virtPortProfile: bridge/port specific configuration
> > <at> <at> -265,11 +267,14 <at> <at> int virNetDevTapDelete(const char *ifname
> ATTRIBUTE_UNUSED)
> > int virNetDevTapCreateInBridgePort(const char *brname,
> > char **ifname,
> > const unsigned char *macaddr,
> > + bool discourage,
> > int vnet_hdr,
> > bool up,
> > int *tapfd,
> > virNetDevVPortProfilePtr
> virtPortProfile)
> > {
> > + unsigned char tapmac[VIR_MAC_BUFLEN];
> > +
> > if (virNetDevTapCreate(ifname, vnet_hdr, tapfd) < 0)
> > return -1;
> >
> > <at> <at> -279,7 +284,11 <at> <at> int virNetDevTapCreateInBridgePort(const
> char *brname,
> > * seeing the kernel allocate random MAC for the TAP
> > * device before we set our static MAC.
> > */
> > - if (virNetDevSetMAC(*ifname, macaddr) < 0)
> > + memcpy(tapmac, macaddr, VIR_MAC_BUFLEN);
> > + if (discourage)
> > + tapmac[0] = 0xFE; /* Discourage bridge from using TAP
> dev MAC */
> > +
> > + if (virNetDevSetMAC(*ifname, tapmac) < 0)
> > goto error;
> >
> > /* We need to set the interface MTU before adding it
> > diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
> > index 918f3dc..fc50e22 100644
> > --- a/src/util/virnetdevtap.h
> > +++ b/src/util/virnetdevtap.h
> > <at> <at> -37,6 +37,7 <at> <at> int virNetDevTapDelete(const char *ifname)
> > int virNetDevTapCreateInBridgePort(const char *brname,
> > char **ifname,
> > const unsigned char *macaddr,
> > + bool discourage,
> > int vnet_hdr,
> > bool up,
> > int *tapfd,
>
> --
> libvir-list mailing list
> https://www.redhat.com/mailman/listinfo/libvir-list>
>
--
libvir-list mailing list
libvir-list <at> redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
<div>
<br><br><div class="gmail_quote">On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <span dir="ltr"><<a href="mailto:laine <at> laine.org">laine <at> laine.org</a>></span> wrote:<br><blockquote class="gmail_quote">
<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 <at> laine.org">laine <at> laine.org</a><br>
</div>
<div class="im">> <mailto:<a href="mailto:laine <at> laine.org">laine <at> 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">
<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>
> > <at> <at> -1766,7 +1766,7 <at> <at> 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>
> > <at> <at> -180,7 +180,6 <at> <at> 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>
> > <at> <at> -244,9 +243,7 <at> <at> 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>
> > <at> <at> -127,7 +127,6 <at> <at> 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>
> > <at> <at> -139,9 +138,7 <at> <at> 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>
> > <at> <at> -22,6 +22,7 <at> <at> <br>
> ><br>
> > #include <config.h><br>
> ><br>
> > +#include "virmacaddr.h"<br>
> > #include "virnetdevtap.h"<br>
> > #include "virnetdev.h"<br>
> > #include "virnetdevbridge.h"<br>
> > <at> <at> -248,6 +249,7 <at> <at> int virNetDevTapDelete(const char *ifname<br>
> ATTRIBUTE_UNUSED)<br>
> > * <at> brname: the bridge name<br>
> > * <at> ifname: the interface name (or name template)<br>
> > * <at> macaddr: desired MAC address (VIR_MAC_BUFLEN long)<br>
> > + * <at> discourage: whether bridge should be discouraged from using<br>
> macaddr<br>
> > * <at> vnet_hdr: whether to try enabling IFF_VNET_HDR<br>
> > * <at> tapfd: file descriptor return value for the new tap device<br>
> > * <at> virtPortProfile: bridge/port specific configuration<br>
> > <at> <at> -265,11 +267,14 <at> <at> 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>
> > <at> <at> -279,7 +284,11 <at> <at> 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>
> > <at> <at> -37,6 +37,7 <at> <at> 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 <at> redhat.com">libvir-list <at> redhat.com</a> <mailto:<a href="mailto:libvir-list <at> redhat.com">libvir-list <at> 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 <at> redhat.com">libvir-list <at> 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>
</div>
Grab me on IRC or by email if you notice somethings which doesn't
work right (the search is not setup yet and I need to reaactivate
various crons, but nearly everything should just work (TM))
Daniel
RSS Feed