On Wed, Feb 09, 2011 at 04:53:32AM -0500, Laine Stump wrote: > 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; > + } I'd be inclined to say that the MAC address should be in the standard XML format we use elsewhere, eg a subelement <mac address='ab:bb:cc:dd:ee:ff'/> > 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); Not sure about the name suffix here, how about '-gw' or '-nic' ? > + if (!macTapIfName) { > + virReportOOMError(); > + goto err0; > + } > + if (!network->def->mac_specified) { > + virGenerateMacAddr((unsigned char[]){ 0x52, 0x54, 0 }, > + network->def->mac); > + network->def->mac_specified = true; > + } As you mentioned above I think this is the wrong place to be generating this because to properly solve the bug above we want the MAC to be stable forever & we require that libvirtd can be restarted without loosing data. I think we should do the auto-generation in the 'Define' and 'Create' methods just afer we've parsed the initial XML. We can also auto-generate in the method which loads configs off disk as a safety net for upgrades. To make this persistent though, we should write a %post script for the libvirt RPM that fixes up any existing deployed networks in /etc/libvirt Regards, 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list