[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] Give each virtual network bridge its own fixed MAC address



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]