[libvirt] [PATCH] Use the same MAC address that is defined in domain XML for attached-mac field.
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. --- 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 @@ -1766,7 +1766,7 @@ 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 @@ -180,7 +180,6 @@ 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) { @@ -244,9 +243,7 @@ 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 @@ -127,7 +127,6 @@ 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) || @@ -139,9 +138,7 @@ 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 @@ -22,6 +22,7 @@ #include <config.h> +#include "virmacaddr.h" #include "virnetdevtap.h" #include "virnetdev.h" #include "virnetdevbridge.h" @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) + * @discourage: whether bridge should be discouraged from using macaddr * @vnet_hdr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device * @virtPortProfile: bridge/port specific configuration @@ -265,11 +267,14 @@ 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; @@ -279,7 +284,11 @@ 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 @@ -37,6 +37,7 @@ 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, -- 1.7.5.4
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. 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 @@ -1766,7 +1766,7 @@ 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 @@ -180,7 +180,6 @@ 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) { @@ -244,9 +243,7 @@ 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 @@ -127,7 +127,6 @@ 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) || @@ -139,9 +138,7 @@ 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 @@ -22,6 +22,7 @@
#include <config.h>
+#include "virmacaddr.h" #include "virnetdevtap.h" #include "virnetdev.h" #include "virnetdevbridge.h" @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) + * @discourage: whether bridge should be discouraged from using macaddr * @vnet_hdr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device * @virtPortProfile: bridge/port specific configuration @@ -265,11 +267,14 @@ 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;
@@ -279,7 +284,11 @@ 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 @@ -37,6 +37,7 @@ 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,
On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <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) 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 @@ -1766,7 +1766,7 @@ 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 @@ -180,7 +180,6 @@ 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) { @@ -244,9 +243,7 @@ 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 @@ -127,7 +127,6 @@ 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) || @@ -139,9 +138,7 @@ 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 @@ -22,6 +22,7 @@
#include <config.h>
+#include "virmacaddr.h" #include "virnetdevtap.h" #include "virnetdev.h" #include "virnetdevbridge.h" @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname
ATTRIBUTE_UNUSED)
* @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) + * @discourage: whether bridge should be discouraged from using macaddr * @vnet_hdr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device * @virtPortProfile: bridge/port specific configuration @@ -265,11 +267,14 @@ 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;
@@ -279,7 +284,11 @@ 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 @@ -37,6 +37,7 @@ 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 libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/17/2012 02:51 PM, Ansis Atteka wrote:
On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <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).) Let's let this sit over the weekend and see if anyone else has a brilliant idea/insight.
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 > @@ -1766,7 +1766,7 @@ 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 > @@ -180,7 +180,6 @@ 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) { > @@ -244,9 +243,7 @@ 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 > @@ -127,7 +127,6 @@ 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) || > @@ -139,9 +138,7 @@ 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 > @@ -22,6 +22,7 @@ > > #include <config.h> > > +#include "virmacaddr.h" > #include "virnetdevtap.h" > #include "virnetdev.h" > #include "virnetdevbridge.h" > @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) > * @brname: the bridge name > * @ifname: the interface name (or name template) > * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) > + * @discourage: whether bridge should be discouraged from using macaddr > * @vnet_hdr: whether to try enabling IFF_VNET_HDR > * @tapfd: file descriptor return value for the new tap device > * @virtPortProfile: bridge/port specific configuration > @@ -265,11 +267,14 @@ 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; > > @@ -279,7 +284,11 @@ 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 > @@ -37,6 +37,7 @@ 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 libvir-list@redhat.com <mailto:libvir-list@redhat.com> https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Feb 18, 2012 at 10:07:45PM -0500, Laine Stump wrote:
On 02/17/2012 02:51 PM, Ansis Atteka wrote:
On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <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).)
Let's let this sit over the weekend and see if anyone else has a brilliant idea/insight.
The reason for setting the top byte to 0xFE, was that the bridge device will initialize its own MAC address, to the numerically lowest MAC address of all interfaces that are attached. Every time the bridge MAC address changes, we got a network blackout of something like 30-60 seconds IIRC. Setting the guest MAC address to 0xFE ensured that the bridge acquired the MAC address of a physical NIC, and ignored the guest NICs. So before we can go with this patch we need to determine what the OpenVSwitch behaviour is wrt bridge MAC addresses, otherwise this patch could be just reintroducing the problem we solved with this 0xFE byte setting. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Mon, Feb 20, 2012 at 1:46 AM, Daniel P. Berrange <berrange@redhat.com>wrote:
On Sat, Feb 18, 2012 at 10:07:45PM -0500, Laine Stump wrote:
On 02/17/2012 02:51 PM, Ansis Atteka wrote:
On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <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).)
Let's let this sit over the weekend and see if anyone else has a brilliant idea/insight.
The reason for setting the top byte to 0xFE, was that the bridge device will initialize its own MAC address, to the numerically lowest MAC address of all interfaces that are attached. Every time the bridge MAC address changes, we got a network blackout of something like 30-60 seconds IIRC. Setting the guest MAC address to 0xFE ensured that the bridge acquired the MAC address of a physical NIC, and ignored the guest NICs.
This behavior applies to OVS as well. So we must set MAC address top byte to 0xFE also for TAP devices that get attached to OVS bridges.
So before we can go with this patch we need to determine what the OpenVSwitch behaviour is wrt bridge MAC addresses, otherwise this patch could be just reintroducing the problem we solved with this 0xFE byte setting.
Sorry for the confusion. I think I needed to to make it clear that this "attached-mac" address has nothing to do with MAC address that gets assigned to the TAP devices. After applying this patch the TAP devices would still get the same FE:XX:XX:XX:XX:XX MAC addresses (irrelevant whether Linux Bridge or OVS bridge). The only thing that would change after applying this patch would be that "attached-mac" (see where we invoke ovs-vsctl command) would match the original MAC address that was defined in Domain XML file. By the way, the only consumer of this "attached-mac" is OpenFlow Controller, which must see the MAC address that is being used by the guest and not the one that gets assigned to the TAP device with highest byte set to 0xFE.
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:| |: http://libvirt.org -o- http://virt-manager.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Feb 21, 2012 at 11:53 AM, Ansis Atteka <aatteka@nicira.com> wrote:
On Mon, Feb 20, 2012 at 1:46 AM, Daniel P. Berrange <berrange@redhat.com>wrote:
On Sat, Feb 18, 2012 at 10:07:45PM -0500, Laine Stump wrote:
On 02/17/2012 02:51 PM, Ansis Atteka wrote:
On Fri, Feb 17, 2012 at 10:55 AM, Laine Stump <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).)
Let's let this sit over the weekend and see if anyone else has a brilliant idea/insight.
The reason for setting the top byte to 0xFE, was that the bridge device will initialize its own MAC address, to the numerically lowest MAC address of all interfaces that are attached. Every time the bridge MAC address changes, we got a network blackout of something like 30-60 seconds IIRC. Setting the guest MAC address to 0xFE ensured that the bridge acquired the MAC address of a physical NIC, and ignored the guest NICs.
This behavior applies to OVS as well. So we must set MAC address top byte to 0xFE also for TAP devices that get attached to OVS bridges.
So before we can go with this patch we need to determine what the OpenVSwitch behaviour is wrt bridge MAC addresses, otherwise this patch could be just reintroducing the problem we solved with this 0xFE byte setting.
Sorry for the confusion. I think I needed to to make it clear that this "attached-mac" address has nothing to do with MAC address that gets assigned to the TAP devices.
After applying this patch the TAP devices would still get the same FE:XX:XX:XX:XX:XX MAC addresses (irrelevant whether Linux Bridge or OVS bridge). The only thing that would change after applying this patch would be that "attached-mac" (see where we invoke ovs-vsctl command) would match the original MAC address that was defined in Domain XML file.
By the way, the only consumer of this "attached-mac" is OpenFlow Controller, which must see the MAC address that is being used by the guest and not the one that gets assigned to the TAP device with highest byte set to 0xFE.
Wanted to bump up this patch to see if there are still any concerns? Not sure if I am missing something, but all this patch does is set the external-ids:attached-mac field to the same MAC address that guest will be using. It should not interfere with what MAC address gets assigned to TAP devices or Bridges on Hypervisor.
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <laine@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@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). 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.
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 > @@ -1766,7 +1766,7 @@ 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 > @@ -180,7 +180,6 @@ 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) { > @@ -244,9 +243,7 @@ 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 > @@ -127,7 +127,6 @@ 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) || > @@ -139,9 +138,7 @@ 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 > @@ -22,6 +22,7 @@ > > #include <config.h> > > +#include "virmacaddr.h" > #include "virnetdevtap.h" > #include "virnetdev.h" > #include "virnetdevbridge.h" > @@ -248,6 +249,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) > * @brname: the bridge name > * @ifname: the interface name (or name template) > * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) > + * @discourage: whether bridge should be discouraged from using macaddr > * @vnet_hdr: whether to try enabling IFF_VNET_HDR > * @tapfd: file descriptor return value for the new tap device > * @virtPortProfile: bridge/port specific configuration > @@ -265,11 +267,14 @@ 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; > > @@ -279,7 +284,11 @@ 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 > @@ -37,6 +37,7 @@ 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 libvir-list@redhat.com <mailto:libvir-list@redhat.com> https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/29/2012 07:26 PM, Ansis Atteka wrote:
On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <laine@laine.org <mailto:laine@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@laine.org <mailto:laine@laine.org> > <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. 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.
On Thu, Mar 1, 2012 at 10:19 AM, Laine Stump <laine@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@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@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:
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.
On 03/01/2012 01:19 PM, Laine Stump wrote:
On 02/29/2012 07:26 PM, Ansis Atteka wrote:
On Sat, Feb 18, 2012 at 7:07 PM, Laine Stump <laine@laine.org <mailto:laine@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@laine.org <mailto:laine@laine.org> > <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.
Ansis - I posted my changes as a separate patch rather than squashed into yours, and posted it. If you're okay with my patch, I'll ACK yours and push them both together. https://www.redhat.com/archives/libvir-list/2012-March/msg00056.html
On Thu, Mar 1, 2012 at 1:17 PM, Laine Stump <laine@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@laine.org <mailto:laine@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@laine.org <mailto:laine@laine.org> > <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
> 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
On 03/01/2012 01:19 PM, Laine Stump wrote: pass 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.
Ansis - I posted my changes as a separate patch rather than squashed into yours, and posted it. If you're okay with my patch, I'll ACK yours and push them both together.
https://www.redhat.com/archives/libvir-list/2012-March/msg00056.html
I am ok with this approach as well.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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.
Okay, after much discussion, and pairing this patch with another that combines the now-three bools in the arglist of virNetDevTapCreateInBridgePort into a single "flags" to make it easier to understand: ACK. I wrote a more descriptive commit notice for this patch (explaining how the whole 0xFE thing works and why), and pushed it together with the aforementioned 3 x bool --> 1 flags patch. Thanks for your patience :-)
participants (3)
-
Ansis Atteka -
Daniel P. Berrange -
Laine Stump