[libvirt] [PATCH 0/2] Give each virtual network bridge its own fixed MAC address
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=609463 As usual for me, this series has a problem searching for a more elegant solution. See PATCH 2/2 for details. Is it too late to put this in 0.8.8? (assuming a solution to that problem, of course).
An upcoming patch has a use for a tap device to be created that doesn't need to be actually put into the "up" state, and keeping it "down" keeps the output of ifconfig from being unnecessarily cluttered (ifconfig won't show down interfaces unless you add "-a"). bridge.[ch]: add "up" as an arg to brAddTap() uml_conf.c, qemu_command.c: add "up" (set to "true") to brAddTap() call. --- src/qemu/qemu_command.c | 1 + src/uml/uml_conf.c | 1 + src/util/bridge.c | 3 ++- src/util/bridge.h | 1 + 4 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f78ce71..600830a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -252,6 +252,7 @@ qemuNetworkIfaceConnect(virConnectPtr conn, &net->ifname, tapmac, vnet_hdr, + true, &tapfd))) { if (err == ENOTSUP) { /* In this particular case, give a better diagnostic. */ diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index e5dbed9..7c8fb16 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -143,6 +143,7 @@ umlConnectTapDevice(virConnectPtr conn, &net->ifname, tapmac, 0, + true, NULL))) { if (err == ENOTSUP) { /* In this particular case, give a better diagnostic. */ diff --git a/src/util/bridge.c b/src/util/bridge.c index e53fce5..3ed71be 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -484,6 +484,7 @@ brAddTap(brControl *ctl, char **ifname, const unsigned char *macaddr, int vnet_hdr, + bool up, int *tapfd) { int fd; @@ -530,7 +531,7 @@ brAddTap(brControl *ctl, goto error; if ((errno = brAddInterface(ctl, bridge, ifr.ifr_name))) goto error; - if ((errno = brSetInterfaceUp(ctl, ifr.ifr_name, 1))) + if (up && ((errno = brSetInterfaceUp(ctl, ifr.ifr_name, 1)))) goto error; if (!tapfd && (errno = ioctl(fd, TUNSETPERSIST, 1))) diff --git a/src/util/bridge.h b/src/util/bridge.h index e8e7eca..93f0b33 100644 --- a/src/util/bridge.h +++ b/src/util/bridge.h @@ -71,6 +71,7 @@ int brAddTap (brControl *ctl, char **ifname, const unsigned char *macaddr, int vnet_hdr, + bool up, int *tapfd); int brDeleteTap (brControl *ctl, -- 1.7.3.4
On Wed, Feb 09, 2011 at 04:53:31AM -0500, Laine Stump wrote:
An upcoming patch has a use for a tap device to be created that doesn't need to be actually put into the "up" state, and keeping it "down" keeps the output of ifconfig from being unnecessarily cluttered (ifconfig won't show down interfaces unless you add "-a").
bridge.[ch]: add "up" as an arg to brAddTap() uml_conf.c, qemu_command.c: add "up" (set to "true") to brAddTap() call. --- src/qemu/qemu_command.c | 1 + src/uml/uml_conf.c | 1 + src/util/bridge.c | 3 ++- src/util/bridge.h | 1 + 4 files changed, 5 insertions(+), 1 deletions(-)
ACK 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 :|
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
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 :|
==================================== Changes in V2: 1) auto-generate mac addresses in networkDefine and networkCreate instead of networkStartNetworkDaemon, and add the %post script to fixup existing networks. 2) instead of mac being an attribute of the <bridge> element, make it its own element, as is done elsewhere in the xml, eg: <bridge name='virbr0' ... /> <mac address='52:54:00:3A:34:16'/> 3) The dummy tap device is now called "<brname>-nic" rather than "<brname>-mac". 4) add <mac address='...'/> to the website xml docs 5) update network.rng 6) add <mac address='...'/> to a couple of the network xml test data files. ==================================== 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-nic" displayed in the output of the ifconfig command. 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 defined, if there is no MAC, one is generated. To account for virtual network configs that already exist when upgrading from an older version of libvirt, I've added a %post script to the specfile that searches for all network definitions in both the config directory (/etc/libvirt/qemu/networks) and the state directory (/var/lib/libvirt/network) that are missing a mac address, generates a random address, and adds it to the config (and a matching address to the state file, if there is one). docs/formatnetwork.html.in: document <mac address.../> docs/schemas/network.rng: add nac address to schema libvirt.spec.in: %post script to update existing networks src/conf/network_conf.[ch]: parse and format <mac address.../> src/libvirt_private.syms: export a couple private symbols we need src/network/bridge_driver.c: auto-generate mac address when needed, create dummy interface if mac address is present. tests/networkxml2xmlin/isolated-network.xml tests/networkxml2xmlin/routed-network.xml tests/networkxml2xmlout/isolated-network.xml tests/networkxml2xmlout/routed-network.xml: add mac address to some tests --- docs/formatnetwork.html.in | 21 ++++++++- docs/schemas/network.rng | 8 +++ libvirt.spec.in | 38 +++++++++++++++ src/conf/network_conf.c | 30 ++++++++++++ src/conf/network_conf.h | 5 ++ src/libvirt_private.syms | 2 + src/network/bridge_driver.c | 65 ++++++++++++++++++++++++++ tests/networkxml2xmlin/isolated-network.xml | 1 + tests/networkxml2xmlin/routed-network.xml | 1 + tests/networkxml2xmlout/isolated-network.xml | 1 + tests/networkxml2xmlout/routed-network.xml | 1 + 11 files changed, 171 insertions(+), 2 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index b1b0485..be5ceb5 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -105,12 +105,15 @@ <h3><a name="elementsAddress">Addressing</a></h3> <p> - The final set of elements define the IPv4 address range available, - and optionally enable DHCP sevices. + The final set of elements define the addresses (IPv4 and/or + IPv6, as well as MAC) to be assigned to the bridge device + associated with the virtual network, and optionally enable DHCP + sevices. </p> <pre> ... + <mac address='00:16:3E:5D:C7:9E'/> <ip address="192.168.122.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.122.100" end="192.168.122.254" /> @@ -121,6 +124,20 @@ </network></pre> <dl> + <dt><code>mac</code></dt> + <dd>The <code>address</code> attribute defines a MAC + (hardware) address formatted as 6 groups of 2-digit + hexadecimal numbers, the groups separated by colons + (eg, <code>"52:54:00:1C:DA:2F"</code>). This MAC address is + assigned to the bridge device when it is created. Generally + it is best to not specify a mac address when creating a + network - in this case, if a defined mac address is needed for + proper operation, libvirt will automatically generate a random + mac address and save it in the config. Allowing libvirt to + generate the MAC address will assure that it is compatible + with the idiosyncracies of the platform where libvirt is + running. <span class="since">Since 0.8.8</span> + </dd> <dt><code>ip</code></dt> <dd>The <code>address</code> attribute defines an IPv4 address in dotted-decimal format, or an IPv6 address in standard diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4252f30..6d01b06 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -50,6 +50,14 @@ </element> </optional> + <!-- <mac> element --> + <optional> + <element name="mac"> + <attribute name="address"><ref name="mac-addr"/></attribute> + <empty/> + </element> + </optional> + <!-- <forward> element --> <optional> <!-- The device through which the bridge is connected to the diff --git a/libvirt.spec.in b/libvirt.spec.in index 0a2d10e..cefdc2c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -741,6 +741,44 @@ then > %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml fi + +# All newly defined networks will have a mac address for the bridge +# auto-generated, but networks already existing at the time of upgrade +# will not. We need to go through all the network configs, look for +# those that don't have a mac address, and add one. + +network_files=`(cd %{_localstatedir}/lib/libvirt/network; \ + ls *.xml | xargs grep -L "mac address"; \ + cd %{_sysconfdir}/libvirt/qemu/networks; \ + ls *.xml | xargs grep -L "mac address") \ + | sort | uniq` + +for file in $network_files +do + # each file exists in either the config or state directory (or both) and + # does not have a mac address specified in either. We add the same mac + # address to both files (or just one, if the other isn't there) + + mac4=`printf '%X' $(($RANDOM % 256))` + mac5=`printf '%X' $(($RANDOM % 256))` + mac6=`printf '%X' $(($RANDOM % 256))` + for dir in %{_localstatedir}/lib/libvirt/network %{_sysconfdir}/libvirt/qemu/networks + do + if test -f $dir/$file + then + sed -i.orig -e \ + "s|\(<bridge.*$\)|\0\n <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if ! test $? + then + echo "failed to add <mac address='52:54:00:$mac4:$mac5:$mac6'/> to $dir/$file" + mv -f $dir/$file.orig $dir/$file + else + rm -f $dir/$file.orig + fi + fi + done +done %endif %if %{with_cgconfig} diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 28a3ee8..dcab9de 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(./mac[1]/@address)", 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; @@ -856,6 +869,11 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferVSprintf(&buf, " stp='%s' delay='%ld' />\n", def->stp ? "on" : "off", def->delay); + if (def->mac_specified) { + char macaddr[VIR_MAC_STRING_BUFLEN]; + virFormatMacAddr(def->mac, macaddr); + virBufferVSprintf(&buf, " <mac address='%s'/>\n", macaddr); + } if (def->domain) virBufferVSprintf(&buf, " <domain name='%s'/>\n", def->domain); @@ -1165,6 +1183,18 @@ error: } +void virNetworkSetBridgeMacAddr(virNetworkDefPtr def) +{ + if (!def->mac_specified) { + /* if the bridge doesn't have a mac address explicitly defined, + * autogenerate a random one. + */ + virGenerateMacAddr((unsigned char[]){ 0x52, 0x54, 0 }, + def->mac); + def->mac_specified = true; + } +} + /* * virNetworkObjIsDuplicate: * @doms : virNetworkObjListPtr to search diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index fd96c36..281124b 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 */ @@ -191,6 +194,8 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, virNetworkDefPtr def, int check_collision); +void virNetworkSetBridgeMacAddr(virNetworkDefPtr def); + int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, unsigned int check_active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b9e3efe..7972a4f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -608,6 +608,7 @@ virNetworkObjLock; virNetworkObjUnlock; virNetworkRemoveInactive; virNetworkSaveConfig; +virNetworkSetBridgeMacAddr; virNetworkSetBridgeName; @@ -877,6 +878,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..8820927 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -128,6 +128,15 @@ networkRadvdConfigFileName(const char *netname) return configfile; } +static char * +networkBridgeDummyNicName(const char *brname) +{ + char *nicname; + + virAsprintf(&nicname, "%s-nic", brname); + return nicname; +} + static void networkFindActiveConfigs(struct network_driver *driver) { unsigned int i; @@ -1566,6 +1575,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 +1595,30 @@ networkStartNetworkDaemon(struct network_driver *driver, return -1; } + if (network->def->mac_specified) { + /* To set a mac for the bridge, we need to define a dummy tap + * device, set its mac, then attach it to the bridge. As long + * as its mac address is lower than any other interface that + * gets attached, the bridge will always maintain this mac + * address. + */ + macTapIfName = networkBridgeDummyNicName(network->def->bridge); + if (!macTapIfName) { + virReportOOMError(); + goto err0; + } + 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 +1729,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 +1759,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 +1790,21 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, kill(network->dnsmasqPid, SIGTERM); char ebuf[1024]; + + if (network->def->mac_specified) { + macTapIfName = networkBridgeDummyNicName(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)); @@ -1988,6 +2049,8 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; + virNetworkSetBridgeMacAddr(def); + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; @@ -2029,6 +2092,8 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; + virNetworkSetBridgeMacAddr(def); + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; diff --git a/tests/networkxml2xmlin/isolated-network.xml b/tests/networkxml2xmlin/isolated-network.xml index 507e3bb..0d562ea 100644 --- a/tests/networkxml2xmlin/isolated-network.xml +++ b/tests/networkxml2xmlin/isolated-network.xml @@ -2,6 +2,7 @@ <name>private</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name="virbr2" /> + <mac address='52:54:00:17:3F:37'/> <ip address="192.168.152.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.152.2" end="192.168.152.254" /> diff --git a/tests/networkxml2xmlin/routed-network.xml b/tests/networkxml2xmlin/routed-network.xml index 6634ee8..61d73c0 100644 --- a/tests/networkxml2xmlin/routed-network.xml +++ b/tests/networkxml2xmlin/routed-network.xml @@ -2,6 +2,7 @@ <name>local</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name="virbr1" /> + <mac address='12:34:56:78:9A:BC'/> <forward mode="route" dev="eth1"/> <ip address="192.168.122.1" netmask="255.255.255.0"> </ip> diff --git a/tests/networkxml2xmlout/isolated-network.xml b/tests/networkxml2xmlout/isolated-network.xml index 1d06f19..cc320a9 100644 --- a/tests/networkxml2xmlout/isolated-network.xml +++ b/tests/networkxml2xmlout/isolated-network.xml @@ -2,6 +2,7 @@ <name>private</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name='virbr2' stp='on' delay='0' /> + <mac address='52:54:00:17:3F:37'/> <ip address='192.168.152.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.152.2' end='192.168.152.254' /> diff --git a/tests/networkxml2xmlout/routed-network.xml b/tests/networkxml2xmlout/routed-network.xml index 8f11166..3aa8109 100644 --- a/tests/networkxml2xmlout/routed-network.xml +++ b/tests/networkxml2xmlout/routed-network.xml @@ -3,6 +3,7 @@ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <forward dev='eth1' mode='route'/> <bridge name='virbr1' stp='on' delay='0' /> + <mac address='12:34:56:78:9A:BC'/> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> </network> -- 1.7.3.4
On 02/10/2011 02:57 AM, Laine Stump wrote:
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).
+++ b/docs/formatnetwork.html.in @@ -105,12 +105,15 @@ <h3><a name="elementsAddress">Addressing</a></h3>
<p> - The final set of elements define the IPv4 address range available, - and optionally enable DHCP sevices. + The final set of elements define the addresses (IPv4 and/or + IPv6, as well as MAC) to be assigned to the bridge device + associated with the virtual network, and optionally enable DHCP + sevices.
s/sevices/services/ (pre-existing, but as long as you're touching that sentence...)
<dl> + <dt><code>mac</code></dt> + <dd>The <code>address</code> attribute defines a MAC + (hardware) address formatted as 6 groups of 2-digit + hexadecimal numbers, the groups separated by colons + (eg, <code>"52:54:00:1C:DA:2F"</code>). This MAC address is + assigned to the bridge device when it is created. Generally + it is best to not specify a mac address when creating a
Hmm, for consistency, I'm thinking s/mac/MAC/g
+ network - in this case, if a defined mac address is needed for + proper operation, libvirt will automatically generate a random + mac address and save it in the config. Allowing libvirt to
(3 instances); but feel free to disregard that if you think it results in too many all-caps words.
+ generate the MAC address will assure that it is compatible + with the idiosyncracies of the platform where libvirt is
s/idiosyncracies/idiosyncrasies/
+++ b/libvirt.spec.in @@ -741,6 +741,44 @@ then > %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml fi + +# All newly defined networks will have a mac address for the bridge +# auto-generated, but networks already existing at the time of upgrade +# will not. We need to go through all the network configs, look for +# those that don't have a mac address, and add one. + +network_files=`(cd %{_localstatedir}/lib/libvirt/network; \ + ls *.xml | xargs grep -L "mac address"; \ + cd %{_sysconfdir}/libvirt/qemu/networks; \ + ls *.xml | xargs grep -L "mac address") \ + | sort | uniq`
This leaks a message to stderr if globbing fails: ls: cannot access *.xml: No such file or directory You want to use && rather than ; after cd (in case for some weird reason cd fails, you don't want to be globbing in the wrong location). sort | uniq wastes a process. For that matter, do we really expect the glob to result in so many networks that we exceed ARG_MAX limitations, or can we ditch the xargs process? `` is yucky - let's use $() network_files=$( (cd %{_localstatedir}/lib/libvirt/network && \ grep -L "mac address" *.xml; \ cd %{_sysconfdir}/libvirt/qemu/networks && \ grep -L "mac address" *.xml) 2>/dev/null \ | sort -u)
+ +for file in $network_files +do + # each file exists in either the config or state directory (or both) and + # does not have a mac address specified in either. We add the same mac + # address to both files (or just one, if the other isn't there) + + mac4=`printf '%X' $(($RANDOM % 256))` + mac5=`printf '%X' $(($RANDOM % 256))` + mac6=`printf '%X' $(($RANDOM % 256))` + for dir in %{_localstatedir}/lib/libvirt/network %{_sysconfdir}/libvirt/qemu/networks + do + if test -f $dir/$file + then + sed -i.orig -e \
Is sed -i atomic? I know perl -i is not (that is, perl moves the file to a temporary, the puts output into the original, but there exists a window of time where the original file name refers to an incomplete file). Are we better off skipping -i, and doing sed into a secondary file, and atomically renaming that file over the original if all went well? On the other hand, then you have to start worrying about file permissions being accidentally changed. http://www.pixelbeat.org/docs/unix_file_replacement.html Or do we just give up on the atomicity requirement, and call this method good enough (that is, you are unlikely to be upgrading rpm at the same time that libvirt is trying to read (or worse modify) one of these .xml files).
+ "s|\(<bridge.*$\)|\0\n <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if ! test $?
Won't work. $? is always non-empty, which means test $? is always true, which means ! test $? will always take the else branch. You meant: if test $? != 0 The code looked fine, but I think it would be wise to see a v3 with the spec file cleanups and spelling fixes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 02/10/2011 04:09 PM, Eric Blake wrote:
On 02/10/2011 02:57 AM, Laine Stump wrote:
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).
+++ b/docs/formatnetwork.html.in @@ -105,12 +105,15 @@ <h3><a name="elementsAddress">Addressing</a></h3>
<p> - The final set of elements define the IPv4 address range available, - and optionally enable DHCP sevices. + The final set of elements define the addresses (IPv4 and/or + IPv6, as well as MAC) to be assigned to the bridge device + associated with the virtual network, and optionally enable DHCP + sevices. s/sevices/services/
(pre-existing, but as long as you're touching that sentence...)
Done.
<dl> +<dt><code>mac</code></dt> +<dd>The<code>address</code> attribute defines a MAC + (hardware) address formatted as 6 groups of 2-digit + hexadecimal numbers, the groups separated by colons + (eg,<code>"52:54:00:1C:DA:2F"</code>). This MAC address is + assigned to the bridge device when it is created. Generally + it is best to not specify a mac address when creating a
Hmm, for consistency, I'm thinking s/mac/MAC/g
+ network - in this case, if a defined mac address is needed for + proper operation, libvirt will automatically generate a random + mac address and save it in the config. Allowing libvirt to (3 instances); but feel free to disregard that if you think it results in too many all-caps words.
Consistency is good. I made them all caps.
+ generate the MAC address will assure that it is compatible + with the idiosyncracies of the platform where libvirt is s/idiosyncracies/idiosyncrasies/
Strange - the "c" version is in /usr/share/dict/words, but not in the dictionary.
+++ b/libvirt.spec.in @@ -741,6 +741,44 @@ then > %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml fi + +# All newly defined networks will have a mac address for the bridge +# auto-generated, but networks already existing at the time of upgrade +# will not. We need to go through all the network configs, look for +# those that don't have a mac address, and add one. + +network_files=`(cd %{_localstatedir}/lib/libvirt/network; \ + ls *.xml | xargs grep -L "mac address"; \ + cd %{_sysconfdir}/libvirt/qemu/networks; \ + ls *.xml | xargs grep -L "mac address") \ + | sort | uniq` This leaks a message to stderr if globbing fails:
ls: cannot access *.xml: No such file or directory
You want to use&& rather than ; after cd (in case for some weird reason cd fails, you don't want to be globbing in the wrong location).
sort | uniq wastes a process. For that matter, do we really expect the glob to result in so many networks that we exceed ARG_MAX limitations, or can we ditch the xargs process?
`` is yucky - let's use $()
All good points. I've switched to your version. (this exercise makes me realize how many shell-isms are just wired into my subconscious; for example, I'm so used to the file list for grep coming from something more complex than a simple glob, that I *always* pipe the filelist through xargs to grep even when it's severe overkill, and have never noticed sort -u)
network_files=$( (cd %{_localstatedir}/lib/libvirt/network&& \ grep -L "mac address" *.xml; \ cd %{_sysconfdir}/libvirt/qemu/networks&& \ grep -L "mac address" *.xml) 2>/dev/null \ | sort -u)
+ +for file in $network_files +do + # each file exists in either the config or state directory (or both) and + # does not have a mac address specified in either. We add the same mac + # address to both files (or just one, if the other isn't there) + + mac4=`printf '%X' $(($RANDOM % 256))` + mac5=`printf '%X' $(($RANDOM % 256))` + mac6=`printf '%X' $(($RANDOM % 256))` + for dir in %{_localstatedir}/lib/libvirt/network %{_sysconfdir}/libvirt/qemu/networks + do + if test -f $dir/$file + then + sed -i.orig -e \ Is sed -i atomic? I know perl -i is not (that is, perl moves the file to a temporary, the puts output into the original, but there exists a window of time where the original file name refers to an incomplete file). Are we better off skipping -i, and doing sed into a secondary file, and atomically renaming that file over the original if all went well? On the other hand, then you have to start worrying about file permissions being accidentally changed.
http://www.pixelbeat.org/docs/unix_file_replacement.html
Or do we just give up on the atomicity requirement, and call this method good enough (that is, you are unlikely to be upgrading rpm at the same time that libvirt is trying to read (or worse modify) one of these .xml files).
My thinking was that sed -i is already used elsewhere in the %post script on the same file (and that usage doesn't even make a backup in case the operation somehow fails, but maybe that's overkill too), so it must be good enough for this case too.
+ "s|\(<bridge.*$\)|\0\n<mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if ! test $? Won't work. $? is always non-empty, which means test $? is always true, which means ! test $? will always take the else branch.
You meant:
if test $? != 0
Yes. It was late, and the script ran properly (only because it didn't encounter an error), so I didn't notice.
The code looked fine, but I think it would be wise to see a v3 with the spec file cleanups and spelling fixes.
Thanks for the (as usual) thorough review! v3 coming up.
Changes in V3 (based on eblake's review of V2): 1) fix typos in formatnetwork.html.in 2) update %post script in libvirt.spec.in per eblake's suggestions All other files are identical to V2 ===================================== 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-nic" displayed in the output of the ifconfig command. 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 defined, if there is no MAC, one is generated. To account for virtual network configs that already exist when upgrading from an older version of libvirt, I've added a %post script to the specfile that searches for all network definitions in both the config directory (/etc/libvirt/qemu/networks) and the state directory (/var/lib/libvirt/network) that are missing a mac address, generates a random address, and adds it to the config (and a matching address to the state file, if there is one). docs/formatnetwork.html.in: document <mac address.../> docs/schemas/network.rng: add nac address to schema libvirt.spec.in: %post script to update existing networks src/conf/network_conf.[ch]: parse and format <mac address.../> src/libvirt_private.syms: export a couple private symbols we need src/network/bridge_driver.c: auto-generate mac address when needed, create dummy interface if mac address is present. tests/networkxml2xmlin/isolated-network.xml tests/networkxml2xmlin/routed-network.xml tests/networkxml2xmlout/isolated-network.xml tests/networkxml2xmlout/routed-network.xml: add mac address to some tests --- docs/formatnetwork.html.in | 21 ++++++++- docs/schemas/network.rng | 8 +++ libvirt.spec.in | 40 ++++++++++++++++ src/conf/network_conf.c | 30 ++++++++++++ src/conf/network_conf.h | 5 ++ src/libvirt_private.syms | 2 + src/network/bridge_driver.c | 65 ++++++++++++++++++++++++++ tests/networkxml2xmlin/isolated-network.xml | 1 + tests/networkxml2xmlin/routed-network.xml | 1 + tests/networkxml2xmlout/isolated-network.xml | 1 + tests/networkxml2xmlout/routed-network.xml | 1 + 11 files changed, 173 insertions(+), 2 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index b1b0485..c6969eb 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -105,12 +105,15 @@ <h3><a name="elementsAddress">Addressing</a></h3> <p> - The final set of elements define the IPv4 address range available, - and optionally enable DHCP sevices. + The final set of elements define the addresses (IPv4 and/or + IPv6, as well as MAC) to be assigned to the bridge device + associated with the virtual network, and optionally enable DHCP + services. </p> <pre> ... + <mac address='00:16:3E:5D:C7:9E'/> <ip address="192.168.122.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.122.100" end="192.168.122.254" /> @@ -121,6 +124,20 @@ </network></pre> <dl> + <dt><code>mac</code></dt> + <dd>The <code>address</code> attribute defines a MAC + (hardware) address formatted as 6 groups of 2-digit + hexadecimal numbers, the groups separated by colons + (eg, <code>"52:54:00:1C:DA:2F"</code>). This MAC address is + assigned to the bridge device when it is created. Generally + it is best to not specify a MAC address when creating a + network - in this case, if a defined MAC address is needed for + proper operation, libvirt will automatically generate a random + MAC address and save it in the config. Allowing libvirt to + generate the MAC address will assure that it is compatible + with the idiosyncrasies of the platform where libvirt is + running. <span class="since">Since 0.8.8</span> + </dd> <dt><code>ip</code></dt> <dd>The <code>address</code> attribute defines an IPv4 address in dotted-decimal format, or an IPv6 address in standard diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4252f30..6d01b06 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -50,6 +50,14 @@ </element> </optional> + <!-- <mac> element --> + <optional> + <element name="mac"> + <attribute name="address"><ref name="mac-addr"/></attribute> + <empty/> + </element> + </optional> + <!-- <forward> element --> <optional> <!-- The device through which the bridge is connected to the diff --git a/libvirt.spec.in b/libvirt.spec.in index 0a2d10e..4736b87 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -741,6 +741,46 @@ then > %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml fi + +# All newly defined networks will have a mac address for the bridge +# auto-generated, but networks already existing at the time of upgrade +# will not. We need to go through all the network configs, look for +# those that don't have a mac address, and add one. + +network_files=$( (cd %{_localstatedir}/lib/libvirt/network && \ + grep -L "mac address" *.xml; \ + cd %{_sysconfdir}/libvirt/qemu/networks && \ + grep -L "mac address" *.xml) 2>/dev/null \ + | sort -u) + +for file in $network_files +do + # each file exists in either the config or state directory (or both) and + # does not have a mac address specified in either. We add the same mac + # address to both files (or just one, if the other isn't there) + + mac4=`printf '%X' $(($RANDOM % 256))` + mac5=`printf '%X' $(($RANDOM % 256))` + mac6=`printf '%X' $(($RANDOM % 256))` + for dir in %{_localstatedir}/lib/libvirt/network \ + %{_sysconfdir}/libvirt/qemu/networks + do + if test -f $dir/$file + then + sed -i.orig -e \ + "s|\(<bridge.*$\)|\0\n <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if test $? != 0 + then + echo "failed to add <mac address='52:54:00:$mac4:$mac5:$mac6'/>" \ + " to $dir/$file" + mv -f $dir/$file.orig $dir/$file + else + rm -f $dir/$file.orig + fi + fi + done +done %endif %if %{with_cgconfig} diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 28a3ee8..dcab9de 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(./mac[1]/@address)", 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; @@ -856,6 +869,11 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferVSprintf(&buf, " stp='%s' delay='%ld' />\n", def->stp ? "on" : "off", def->delay); + if (def->mac_specified) { + char macaddr[VIR_MAC_STRING_BUFLEN]; + virFormatMacAddr(def->mac, macaddr); + virBufferVSprintf(&buf, " <mac address='%s'/>\n", macaddr); + } if (def->domain) virBufferVSprintf(&buf, " <domain name='%s'/>\n", def->domain); @@ -1165,6 +1183,18 @@ error: } +void virNetworkSetBridgeMacAddr(virNetworkDefPtr def) +{ + if (!def->mac_specified) { + /* if the bridge doesn't have a mac address explicitly defined, + * autogenerate a random one. + */ + virGenerateMacAddr((unsigned char[]){ 0x52, 0x54, 0 }, + def->mac); + def->mac_specified = true; + } +} + /* * virNetworkObjIsDuplicate: * @doms : virNetworkObjListPtr to search diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index fd96c36..281124b 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 */ @@ -191,6 +194,8 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, virNetworkDefPtr def, int check_collision); +void virNetworkSetBridgeMacAddr(virNetworkDefPtr def); + int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, unsigned int check_active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b9e3efe..7972a4f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -608,6 +608,7 @@ virNetworkObjLock; virNetworkObjUnlock; virNetworkRemoveInactive; virNetworkSaveConfig; +virNetworkSetBridgeMacAddr; virNetworkSetBridgeName; @@ -877,6 +878,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..8820927 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -128,6 +128,15 @@ networkRadvdConfigFileName(const char *netname) return configfile; } +static char * +networkBridgeDummyNicName(const char *brname) +{ + char *nicname; + + virAsprintf(&nicname, "%s-nic", brname); + return nicname; +} + static void networkFindActiveConfigs(struct network_driver *driver) { unsigned int i; @@ -1566,6 +1575,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 +1595,30 @@ networkStartNetworkDaemon(struct network_driver *driver, return -1; } + if (network->def->mac_specified) { + /* To set a mac for the bridge, we need to define a dummy tap + * device, set its mac, then attach it to the bridge. As long + * as its mac address is lower than any other interface that + * gets attached, the bridge will always maintain this mac + * address. + */ + macTapIfName = networkBridgeDummyNicName(network->def->bridge); + if (!macTapIfName) { + virReportOOMError(); + goto err0; + } + 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 +1729,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 +1759,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 +1790,21 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, kill(network->dnsmasqPid, SIGTERM); char ebuf[1024]; + + if (network->def->mac_specified) { + macTapIfName = networkBridgeDummyNicName(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)); @@ -1988,6 +2049,8 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; + virNetworkSetBridgeMacAddr(def); + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; @@ -2029,6 +2092,8 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { if (virNetworkSetBridgeName(&driver->networks, def, 1)) goto cleanup; + virNetworkSetBridgeMacAddr(def); + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; diff --git a/tests/networkxml2xmlin/isolated-network.xml b/tests/networkxml2xmlin/isolated-network.xml index 507e3bb..0d562ea 100644 --- a/tests/networkxml2xmlin/isolated-network.xml +++ b/tests/networkxml2xmlin/isolated-network.xml @@ -2,6 +2,7 @@ <name>private</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name="virbr2" /> + <mac address='52:54:00:17:3F:37'/> <ip address="192.168.152.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.152.2" end="192.168.152.254" /> diff --git a/tests/networkxml2xmlin/routed-network.xml b/tests/networkxml2xmlin/routed-network.xml index 6634ee8..61d73c0 100644 --- a/tests/networkxml2xmlin/routed-network.xml +++ b/tests/networkxml2xmlin/routed-network.xml @@ -2,6 +2,7 @@ <name>local</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name="virbr1" /> + <mac address='12:34:56:78:9A:BC'/> <forward mode="route" dev="eth1"/> <ip address="192.168.122.1" netmask="255.255.255.0"> </ip> diff --git a/tests/networkxml2xmlout/isolated-network.xml b/tests/networkxml2xmlout/isolated-network.xml index 1d06f19..cc320a9 100644 --- a/tests/networkxml2xmlout/isolated-network.xml +++ b/tests/networkxml2xmlout/isolated-network.xml @@ -2,6 +2,7 @@ <name>private</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <bridge name='virbr2' stp='on' delay='0' /> + <mac address='52:54:00:17:3F:37'/> <ip address='192.168.152.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.152.2' end='192.168.152.254' /> diff --git a/tests/networkxml2xmlout/routed-network.xml b/tests/networkxml2xmlout/routed-network.xml index 8f11166..3aa8109 100644 --- a/tests/networkxml2xmlout/routed-network.xml +++ b/tests/networkxml2xmlout/routed-network.xml @@ -3,6 +3,7 @@ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> <forward dev='eth1' mode='route'/> <bridge name='virbr1' stp='on' delay='0' /> + <mac address='12:34:56:78:9A:BC'/> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> </network> -- 1.7.3.4
On 02/11/2011 01:41 AM, Laine Stump wrote:
Changes in V3 (based on eblake's review of V2):
1) fix typos in formatnetwork.html.in
2) update %post script in libvirt.spec.in per eblake's suggestions
All other files are identical to V2
docs/formatnetwork.html.in: document <mac address.../> docs/schemas/network.rng: add nac address to schema libvirt.spec.in: %post script to update existing networks src/conf/network_conf.[ch]: parse and format <mac address.../> src/libvirt_private.syms: export a couple private symbols we need src/network/bridge_driver.c: auto-generate mac address when needed, create dummy interface if mac address is present. tests/networkxml2xmlin/isolated-network.xml tests/networkxml2xmlin/routed-network.xml tests/networkxml2xmlout/isolated-network.xml tests/networkxml2xmlout/routed-network.xml: add mac address to some tests
+ if test -f $dir/$file + then + sed -i.orig -e \ + "s|\(<bridge.*$\)|\0\n <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if test $? != 0 + then + echo "failed to add <mac address='52:54:00:$mac4:$mac5:$mac6'/>" \ + " to $dir/$file"
This message will have two spaces (echo has two arguments, so it separates them with space; and the second argument has a leading space). s/" /"/ ACK with that nit fixed; however, I'm not sure if this qualifies as a bug fix for 0.8.8 or a feature for post-0.8.8. DV? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Feb 11, 2011 at 11:33:49AM -0700, Eric Blake wrote:
On 02/11/2011 01:41 AM, Laine Stump wrote:
Changes in V3 (based on eblake's review of V2):
1) fix typos in formatnetwork.html.in
2) update %post script in libvirt.spec.in per eblake's suggestions
All other files are identical to V2
docs/formatnetwork.html.in: document <mac address.../> docs/schemas/network.rng: add nac address to schema libvirt.spec.in: %post script to update existing networks src/conf/network_conf.[ch]: parse and format <mac address.../> src/libvirt_private.syms: export a couple private symbols we need src/network/bridge_driver.c: auto-generate mac address when needed, create dummy interface if mac address is present. tests/networkxml2xmlin/isolated-network.xml tests/networkxml2xmlin/routed-network.xml tests/networkxml2xmlout/isolated-network.xml tests/networkxml2xmlout/routed-network.xml: add mac address to some tests
+ if test -f $dir/$file + then + sed -i.orig -e \ + "s|\(<bridge.*$\)|\0\n <mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ + $dir/$file + if test $? != 0 + then + echo "failed to add <mac address='52:54:00:$mac4:$mac5:$mac6'/>" \ + " to $dir/$file"
This message will have two spaces (echo has two arguments, so it separates them with space; and the second argument has a leading space). s/" /"/
ACK with that nit fixed; however, I'm not sure if this qualifies as a bug fix for 0.8.8 or a feature for post-0.8.8. DV?
I'm fine to roll this in 0.8.8, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump