[libvirt] [PATCH 2/2] Give each virtual network bridge its own fixed MAC address
Daniel P. Berrange
berrange at redhat.com
Wed Feb 9 13:46:54 UTC 2011
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 :|
More information about the libvir-list
mailing list