3:53 a.m.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=609463
The problem was that, since a bridge always acquires the MAC address
of the connected interface with the numerically lowest MAC, as guests
are started and stopped, it was possible for the MAC address to change
over time, and this change in the network was being detected by
Windows 7 (it sees the MAC of the default route change), so on each
reboot it would bring up a dialog box asking about this "new network".
The solution is to create a dummy tap interface with a MAC guaranteed
to be lower than any guest interface's MAC, and attach that tap to the
bridge as soon as it's created. Since all guest MAC addresses start
with 0xFE, we can just generate a MAC with the standard "0x52, 0x54,
0" prefix, and it's guaranteed to always win (physical interfaces are
never connected to these bridges, so we don't need to worry about
competing numerically with them).
Note that the dummy tap is never set to IFF_UP state - that's not
necessary in order for the bridge to take its MAC, and not setting it
to UP eliminates the clutter of having an (eg) "virbr0-mac" displayed
in the output of the ifconfig command.
Problem that needs a solution:
-----------------------------
I chose to not auto-generate the MAC address in the network XML
parser, as there are likely to be consumers of that API that don't
need or want to have a MAC address associated with the
bridge.
Instead, in bridge_driver.c when the network is being brought
up, if there is no MAC, I generate one then. One down side of this is
that the MAC is written to the *statedir* xml file (in
/var/lib/libvirt/networks) but no to the *config* xml (in
/etc/libvirt/qemu/networks). That means that if libvirtd is restarted
while the network is down, the next time it's started it will have a
different MAC.
It looks like the only place the *config* xml is written is in
networkDefine or networkCreate, but that's inadequate for this case,
as anyone upgrading their libvirt will want this change to take effect
for all their already-defined networks.
DV suggested that we could add a flag to the parser telling it whether
or not to auto-generate a MAC when one wasn't specified. That would
require (at least) adding a new arg to:
virNetworkDefParseFile
virNetworkDefParse
virNetworkDefParseNode
virNetworkDefParseXML
and adding this arg would be embedding details of the XML attributes
into the C API arglist, which doesn't seem very clean - this could set
a very bad precedent that would lead to argument clutter (one of the
things that using XML helps to prevent). Also, it would be one more
arg to be potentially set incorrectly by some new user of the parser.
Any advice on the cleanest way to solve this problem?
---
src/conf/network_conf.c | 21 ++++++++++++++++++-
src/conf/network_conf.h | 3 ++
src/libvirt_private.syms | 1 +
src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 71 insertions(+), 1 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 28a3ee8..592e38c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -628,6 +628,19 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay)
< 0)
def->delay = 0;
+ tmp = virXPathString("string(./bridge[1]/@mac)", ctxt);
+ if (tmp) {
+ if (virParseMacAddr(tmp, def->mac) < 0) {
+ virNetworkReportError(VIR_ERR_XML_ERROR,
+ _("Invalid bridge mac address '%s' in
network '%s'"),
+ tmp, def->name);
+ VIR_FREE(tmp);
+ goto error;
+ }
+ VIR_FREE(tmp);
+ def->mac_specified = true;
+ }
+
nIps = virXPathNodeSet("./ip", ctxt, &ipNodes);
if (nIps > 0) {
int ii;
@@ -853,9 +866,15 @@ char *virNetworkDefFormat(const virNetworkDefPtr def)
virBufferAddLit(&buf, " <bridge");
if (def->bridge)
virBufferEscapeString(&buf, " name='%s'", def->bridge);
- virBufferVSprintf(&buf, " stp='%s' delay='%ld'
/>\n",
+ virBufferVSprintf(&buf, " stp='%s' delay='%ld'",
def->stp ? "on" : "off",
def->delay);
+ if (def->mac_specified) {
+ char macaddr[VIR_MAC_STRING_BUFLEN];
+ virFormatMacAddr(def->mac, macaddr);
+ virBufferVSprintf(&buf, " mac='%s'", macaddr);
+ }
+ virBufferAddLit(&buf, " />\n");
if (def->domain)
virBufferVSprintf(&buf, " <domain name='%s'/>\n",
def->domain);
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index fd96c36..1bde4ac 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -31,6 +31,7 @@
# include "internal.h"
# include "threads.h"
# include "network.h"
+# include "util.h"
/* 2 possible types of forwarding */
enum virNetworkForwardType {
@@ -92,6 +93,8 @@ struct _virNetworkDef {
char *domain;
unsigned long delay; /* Bridge forward delay (ms) */
unsigned int stp :1; /* Spanning tree protocol */
+ unsigned char mac[VIR_MAC_BUFLEN]; /* mac address of bridge device */
+ bool mac_specified;
int forwardType; /* One of virNetworkForwardType constants */
char *forwardDev; /* Destination device for forwarding */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 88e270c..68e42d5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -874,6 +874,7 @@ virFileWriteStr;
virFindFileInPath;
virFork;
virFormatMacAddr;
+virGenerateMacAddr;
virGetGroupID;
virGetHostname;
virGetUserDirectory;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 08aaa36..1952dfd 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1566,6 +1566,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
bool v4present = false, v6present = false;
virErrorPtr save_err = NULL;
virNetworkIpDefPtr ipdef;
+ char *macTapIfName;
if (virNetworkObjIsActive(network)) {
networkReportError(VIR_ERR_OPERATION_INVALID,
@@ -1585,6 +1586,27 @@ networkStartNetworkDaemon(struct network_driver *driver,
return -1;
}
+ virAsprintf(&macTapIfName, "%s-mac", network->def->bridge);
+ if (!macTapIfName) {
+ virReportOOMError();
+ goto err0;
+ }
+ if (!network->def->mac_specified) {
+ virGenerateMacAddr((unsigned char[]){ 0x52, 0x54, 0 },
+ network->def->mac);
+ network->def->mac_specified = true;
+ }
+ if ((err = brAddTap(driver->brctl, network->def->bridge,
+ &macTapIfName, network->def->mac, 0, false, NULL))) {
+ virReportSystemError(err,
+ _("cannot create dummy tap device '%s' to set
mac address on bridge '%s'"),
+ macTapIfName, network->def->bridge);
+ VIR_FREE(macTapIfName);
+ goto err0;
+ }
+
+ VIR_FREE(macTapIfName);
+
/* Set bridge options */
if ((err = brSetForwardDelay(driver->brctl, network->def->bridge,
network->def->delay))) {
@@ -1695,6 +1717,17 @@ networkStartNetworkDaemon(struct network_driver *driver,
err1:
if (!save_err)
save_err = virSaveLastError();
+
+ if ((err = brDeleteTap(driver->brctl, macTapIfName))) {
+ char ebuf[1024];
+ VIR_WARN("Failed to delete dummy tap device '%s' on bridge
'%s' : %s",
+ macTapIfName, network->def->bridge,
+ virStrerror(err, ebuf, sizeof ebuf));
+ }
+
+ err0:
+ if (!save_err)
+ save_err = virSaveLastError();
if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) {
char ebuf[1024];
VIR_WARN("Failed to delete bridge '%s' : %s",
@@ -1714,6 +1747,7 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
{
int err;
char *stateFile;
+ char *macTapIfName;
VIR_INFO(_("Shutting down network '%s'"),
network->def->name);
@@ -1744,6 +1778,19 @@ static int networkShutdownNetworkDaemon(struct network_driver
*driver,
kill(network->dnsmasqPid, SIGTERM);
char ebuf[1024];
+
+ virAsprintf(&macTapIfName, "%s-mac", network->def->bridge);
+ if (!macTapIfName) {
+ virReportOOMError();
+ } else {
+ if ((err = brDeleteTap(driver->brctl, macTapIfName))) {
+ VIR_WARN("Failed to delete dummy tap device '%s' on bridge
'%s' : %s",
+ macTapIfName, network->def->bridge,
+ virStrerror(err, ebuf, sizeof ebuf));
+ }
+ VIR_FREE(macTapIfName);
+ }
+
if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) {
VIR_WARN("Failed to bring down bridge '%s' : %s",
network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
--
1.7.3.4