network: move auto-assign of bridge name from XML parser to net driver

We already check that any auto-assigned bridge device name for a
virtual network (e.g. "virbr1") doesn't conflict with the bridge name
for any existing libvirt network (via virNetworkSetBridgeName() in
conf/network_conf.c).

We also want to check that the name doesn't conflict with any bridge
device created on the host system outside the control of libvirt
(history: possibly due to the ploriferation of references to libvirt's
bridge devices in HOWTO documents all around the web, it is not
uncommon for an admin to manually create a bridge in their host's
system network config and name it "virbrX"). To add such a check to
virNetworkBridgeInUse() (which is called by virNetworkSetBridgeName())
we would have to call virNetDevExists() (from util/virnetdev.c); this
function calls ioctl(SIOCGIFFLAGS), which everyone on the mailing list
agreed should not be done from an XML parsing function in the conf
directory.

To remedy that problem, this patch removes virNetworkSetBridgeName()
from conf/network_conf.c and puts an identically functioning
networkBridgeNameValidate() in network/bridge_driver.c (because it's
reasonable for the bridge driver to call virNetDevExists(), although
we don't do that yet because I wanted this patch to have as close to 0
effect on function as possible).

There are a couple of inevitable changes though:

1) We no longer check the bridge name during
   virNetworkLoadConfig(). Close examination of the code shows that
   this wasn't necessary anyway - the only *correct* way to get XML
   into the config files is via networkDefine(), and networkDefine()
   will always call networkValidate(), which previously called
   virNetworkSetBridgeName() (and now calls
   networkBridgeNameValidate()). This means that the only way the
   bridge name can be unset during virNetworkLoadConfig() is if
   someone edited the config file on disk by hand (which we explicitly
   prohibit).

2) Just on the off chance that somebody *has* edited the file by hand,
   rather than crashing when they try to start their malformed
   network, a check for non-NULL bridge name has been added to
   networkStartNetworkVirtual().

   (For those wondering why I don't instead call
   networkValidateBridgeName() there to set a bridge name if one
   wasn't present - the problem is that during
   networkStartNetworkVirtual(), the lock for the network being
   started has already been acquired, but the lock for the network
   list itself *has not* (because we aren't adding/removing a
   network). But virNetworkBridgeInuse() iterates through *all*
   networks (including this one) and locks each network as it is
   checked for a duplicate entry; it is necessary to lock each network
   even before checking if it is the designated "skip" network because
   otherwise some other thread might acquire the list lock and delete
   the very entry we're examining. In the end, permitting a setting of
   the bridge name during network start would require that we lock the
   entire network list during any networkStartNetwork(), which
   eliminates a *lot* of parallelism that we've worked so hard to
   achieve (it can make a huge difference during libvirtd startup). So
   rather than try to adjust for someone playing against the rules, I
   choose to instead give them the error they deserve.)

3) virNetworkAllocateBridge() (now removed) would leak any "template"
   string set as the bridge name. Its replacement
   networkFindUnusedBridgeName() doesn't leak the template string - it
   is properly freed.

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index f4a9df0cf4460a580e20cb0bf08db558cdee80d2..aa8d6c632c51402c2c62780a6f4b7235c6ee7b61 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -45,7 +45,6 @@
 #include "virfile.h"
 #include "virstring.h"
 
-#define MAX_BRIDGE_ID 256
 #define VIR_FROM_THIS VIR_FROM_NETWORK
 /* currently, /sbin/tc implementation allows up to 16 bits for minor class size */
 #define CLASS_ID_BITMAP_SIZE (1<<16)
@@ -3123,11 +3122,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
             virNetworkSetBridgeMacAddr(def);
             virNetworkSaveConfig(configDir, def);
         }
-        /* Generate a bridge if none is specified, but don't check for collisions
-         * if a bridge is hardcoded, so the network is at least defined.
-         */
-        if (virNetworkSetBridgeName(nets, def, 0))
-            goto error;
     } else {
         /* Throw away MAC address for other forward types,
          * which could have been generated by older libvirt RPMs */
@@ -3303,60 +3297,6 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets,
     return obj != NULL;
 }
 
-char *virNetworkAllocateBridge(virNetworkObjListPtr nets,
-                               const char *template)
-{
-
-    int id = 0;
-    char *newname;
-
-    if (!template)
-        template = "virbr%d";
-
-    do {
-        if (virAsprintf(&newname, template, id) < 0)
-            return NULL;
-        if (!virNetworkBridgeInUse(nets, newname, NULL))
-            return newname;
-        VIR_FREE(newname);
-
-        id++;
-    } while (id <= MAX_BRIDGE_ID);
-
-    virReportError(VIR_ERR_INTERNAL_ERROR,
-                   _("Bridge generation exceeded max id %d"),
-                   MAX_BRIDGE_ID);
-    return NULL;
-}
-
-int virNetworkSetBridgeName(virNetworkObjListPtr nets,
-                            virNetworkDefPtr def,
-                            int check_collision)
-{
-    int ret = -1;
-
-    if (def->bridge && !strstr(def->bridge, "%d")) {
-        /* We may want to skip collision detection in this case (ex. when
-         * loading configs at daemon startup, so the network is at least
-         * defined. */
-        if (check_collision &&
-            virNetworkBridgeInUse(nets, def->bridge, def->name)) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("bridge name '%s' already in use."),
-                           def->bridge);
-            goto error;
-        }
-    } else {
-        /* Allocate a bridge name */
-        if (!(def->bridge = virNetworkAllocateBridge(nets, def->bridge)))
-            goto error;
-    }
-
-    ret = 0;
- error:
-    return ret;
-}
-
 
 void virNetworkSetBridgeMacAddr(virNetworkDefPtr def)
 {

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index f69d9999f304fcee22400fda425a0e48f17dba36..9411a02e32cb4d9f687943750b231a2a38c5d18e 100644 (file)
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -1,7 +1,7 @@
 /*
  * network_conf.h: network XML handling
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -401,13 +401,6 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets,
                           const char *bridge,
                           const char *skipname);
 
-char *virNetworkAllocateBridge(virNetworkObjListPtr nets,
-                               const char *template);
-
-int virNetworkSetBridgeName(virNetworkObjListPtr nets,
-                            virNetworkDefPtr def,
-                            int check_collision);
-
 void virNetworkSetBridgeMacAddr(virNetworkDefPtr def);
 
 int

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3a998134e26d03f0efc309ccbbd966aef78a9cff..c8e6fb4d5539c959c42bd0ee7dbe3a61f7c58dc4 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -576,6 +576,7 @@ virNetDevVPortTypeToString;
 
 # conf/network_conf.h
 virNetworkAssignDef;
+virNetworkBridgeInUse;
 virNetworkBridgeMACTableManagerTypeFromString;
 virNetworkBridgeMACTableManagerTypeToString;
 virNetworkConfigChangeSetup;
@@ -618,7 +619,6 @@ virNetworkRemoveInactive;
 virNetworkSaveConfig;
 virNetworkSaveStatus;
 virNetworkSetBridgeMacAddr;
-virNetworkSetBridgeName;
 virNetworkTaintTypeFromString;
 virNetworkTaintTypeToString;
 virPortGroupFindByName;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d195085dc8f1f57b763dad22c668bf0d8d60aebd..48b9d0fadec64fe7d405abc725dbe648891f4385 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -76,6 +76,7 @@
 #include "virjson.h"
 
 #define VIR_FROM_THIS VIR_FROM_NETWORK
+#define MAX_BRIDGE_ID 256
 
 /**
  * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX:
@@ -449,6 +450,7 @@ networkUpdateState(virNetworkObjPtr obj,
     return ret;
 }
 
+
 static int
 networkAutostartConfig(virNetworkObjPtr net,
                        void *opaque)
@@ -2034,6 +2036,20 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
         return -1;
 
     /* Create and configure the bridge device */
+    if (!network->def->bridge) {
+        /* bridge name can only be empty if the config files were
+         * edited directly. Otherwise networkValidate() (called after
+         * parsing the XML from networkCreateXML() and
+         * networkDefine()) guarantees we will have a valid bridge
+         * name before this point. Since hand editing of the config
+         * files is explicitly prohibited we can, with clear
+         * conscience, log an error and fail at this point.
+         */
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("network '%s' has no bridge name defined"),
+                       network->def->name);
+        return -1;
+    }
     if (virNetDevBridgeCreate(network->def->bridge) < 0)
         return -1;
 
@@ -2746,6 +2762,76 @@ static int networkIsPersistent(virNetworkPtr net)
 }
 
 
+/*
+ * networkFindUnusedBridgeName() - try to find a bridge name that is
+ * unused by the currently configured libvirt networks, and set this
+ * network's name to that new name.
+ */
+static int
+networkFindUnusedBridgeName(virNetworkObjListPtr nets,
+                            virNetworkDefPtr def)
+{
+
+    int ret = -1, id = 0;
+    char *newname = NULL;
+    const char *templ = def->bridge ? def->bridge : "virbr%d";
+
+    do {
+        if (virAsprintf(&newname, templ, id) < 0)
+            goto cleanup;
+        if (!virNetworkBridgeInUse(nets, newname, def->name)) {
+            VIR_FREE(def->bridge); /*could contain template */
+            def->bridge = newname;
+            ret = 0;
+            goto cleanup;
+        }
+        VIR_FREE(newname);
+    } while (++id <= MAX_BRIDGE_ID);
+
+    virReportError(VIR_ERR_INTERNAL_ERROR,
+                   _("Bridge generation exceeded max id %d"),
+                   MAX_BRIDGE_ID);
+    ret = 0;
+ cleanup:
+    if (ret < 0)
+        VIR_FREE(newname);
+    return ret;
+}
+
+
+
+/*
+ * networkValidateBridgeName() - if no bridge name is set, or if the
+ * bridge name contains a %d (indicating that this is a template for
+ * the actual name) try to set an appropriate bridge name.  If a
+ * bridge name *is* set, make sure it doesn't conflict with any other
+ * network's bridge name.
+ */
+static int
+networkBridgeNameValidate(virNetworkObjListPtr nets,
+                          virNetworkDefPtr def)
+{
+    int ret = -1;
+
+    if (def->bridge && !strstr(def->bridge, "%d")) {
+        if (virNetworkBridgeInUse(nets, def->bridge, def->name)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("bridge name '%s' already in use."),
+                           def->bridge);
+            goto cleanup;
+        }
+    } else {
+        /* Allocate a bridge name */
+        if (networkFindUnusedBridgeName(nets, def) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    return ret;
+}
+
+
 static int
 networkValidate(virNetworkDriverStatePtr driver,
                 virNetworkDefPtr def)
@@ -2765,7 +2851,10 @@ networkValidate(virNetworkDriverStatePtr driver,
         def->forward.type == VIR_NETWORK_FORWARD_NAT ||
         def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
 
-        if (virNetworkSetBridgeName(driver->networks, def, 1))
+        /* if no bridge name was given in the config, find a name
+         * unused by any other libvirt networks and assign it.
+         */
+        if (networkBridgeNameValidate(driver->networks, def) < 0)
             return -1;
 
         virNetworkSetBridgeMacAddr(def);