network_conf: Drop virNetworkObjIsDuplicate

This function does not make any sense now, that network driver is
(almost) dropped. I mean, previously, when threads were
serialized, this function was there to check, if no other network
with the same name or UUID exists. However, nowadays that threads
can run more in parallel, this function is useless, in fact it
gives misleading return values. Consider the following scenario.
Two threads, both trying to define networks with same name but
different UUID (e.g. because it was generated during XML parsing
phase, whatever). Lets assume that both threads are about to call
networkValidate() which immediately calls
virNetworkObjIsDuplicate().

T1: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.
T2: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.

T1: calls virNetworkAssignDef() and successfully places its
network into the virNetworkObjList.
T2: calls virNetworkAssignDef() and since network with the same
name exists, the network definition is replaced.

Okay, this is mainly because virNetworkAssignDef() does not check
whether name and UUID matches. Well, lets make it so! And drop
useless function too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

src/conf/network_conf.c

@@ -504,55 +504,108 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
 }
 
 /*
- * virNetworkAssignDef:
- * @nets: list of all networks
- * @def: the new NetworkDef (will be consumed by this function iff successful)
- * @live: is this new def the "live" version, or the "persistent" version
+ * If flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE then this will
+ * refuse updating an existing def if the current def is live
  *
- * Either replace the appropriate copy of the NetworkDef with name
- * matching def->name or, if not found, create a new NetworkObj with
- * def. For an existing network, use "live" and current state of the
- * network to determine which to replace.
+ * If flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE then the @def being
+ * added is assumed to represent a live config, not a future
+ * inactive config
  *
- * Returns NULL on error, virNetworkObjPtr on success.
+ * If flags is zero, network is considered as inactive and persistent.
  */
-virNetworkObjPtr
-virNetworkAssignDef(virNetworkObjListPtr nets,
+static virNetworkObjPtr
+virNetworkAssignDefLocked(virNetworkObjListPtr nets,
                           virNetworkDefPtr def,
-                    bool live)
+                          unsigned int flags)
 {
     virNetworkObjPtr network;
+    virNetworkObjPtr ret = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
 
-    virObjectLock(nets);
-    if ((network = virNetworkObjFindByNameLocked(nets, def->name))) {
-        virObjectUnlock(nets);
+    /* See if a network with matching UUID already exists */
+    if ((network = virNetworkObjFindByUUIDLocked(nets, def->uuid))) {
         virObjectLock(network);
-        virNetworkObjAssignDef(network, def, live);
-        return network;
+        /* UUID matches, but if names don't match, refuse it */
+        if (STRNEQ(network->def->name, def->name)) {
+            virUUIDFormat(network->def->uuid, uuidstr);
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("network '%s' is already defined with uuid %s"),
+                           network->def->name, uuidstr);
+            goto cleanup;
         }
 
-    if (!(network = virNetworkObjNew())) {
-        virObjectUnlock(nets);
-        return NULL;
+        if (flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE) {
+            /* UUID & name match, but if network is already active, refuse it */
+            if (virNetworkObjIsActive(network)) {
+                virReportError(VIR_ERR_OPERATION_INVALID,
+                               _("network is already active as '%s'"),
+                               network->def->name);
+                goto cleanup;
+            }
         }
+
+        virNetworkObjAssignDef(network,
+                               def,
+                               !!(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE));
+    } else {
+        /* UUID does not match, but if a name matches, refuse it */
+        if ((network = virNetworkObjFindByNameLocked(nets, def->name))) {
+            virObjectLock(network);
+            virUUIDFormat(network->def->uuid, uuidstr);
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("network '%s' already exists with uuid %s"),
+                           def->name, uuidstr);
+            goto cleanup;
+        }
+
+        if (!(network = virNetworkObjNew()))
+              goto cleanup;
+
         virObjectLock(network);
 
         virUUIDFormat(def->uuid, uuidstr);
         if (virHashAddEntry(nets->objs, uuidstr, network) < 0)
-        goto error;
+            goto cleanup;
 
         network->def = def;
-    network->persistent = !live;
+        network->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE);
         virObjectRef(network);
-    virObjectUnlock(nets);
-    return network;
+    }
 
- error:
-    virObjectUnlock(nets);
+    ret = network;
+    network = NULL;
+
+ cleanup:
     virNetworkObjEndAPI(&network);
-    return NULL;
+    return ret;
+}
 
+/*
+ * virNetworkAssignDef:
+ * @nets: list of all networks
+ * @def: the new NetworkDef (will be consumed by this function iff successful)
+ * @flags: bitwise-OR of VIR_NETWORK_OBJ_LIST_ADD_* flags
+ *
+ * Either replace the appropriate copy of the NetworkDef with name
+ * matching def->name or, if not found, create a new NetworkObj with
+ * def. For an existing network, use "live" and current state of the
+ * network to determine which to replace.
+ *
+ * Look at virNetworkAssignDefLocked() for @flags description.
+ *
+ * Returns NULL on error, virNetworkObjPtr on success.
+ */
+virNetworkObjPtr
+virNetworkAssignDef(virNetworkObjListPtr nets,
+                    virNetworkDefPtr def,
+                    unsigned int flags)
+{
+    virNetworkObjPtr network;
+
+    virObjectLock(nets);
+    network = virNetworkAssignDefLocked(nets, def, flags);
+    virObjectUnlock(nets);
+    return network;
 }
 
 /*
@@ -3005,7 +3058,7 @@ virNetworkLoadState(virNetworkObjListPtr nets,
     }
 
     /* create the object */
-    if (!(net = virNetworkAssignDef(nets, def, true)))
+    if (!(net = virNetworkAssignDef(nets, def, VIR_NETWORK_OBJ_LIST_ADD_LIVE)))
         goto error;
     /* do not put any "goto error" below this comment */
 
@@ -3082,7 +3135,7 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
         def->mac_specified = false;
     }
 
-    if (!(net = virNetworkAssignDef(nets, def, false)))
+    if (!(net = virNetworkAssignDef(nets, def, 0)))
         goto error;
 
     net->autostart = autostart;
@@ -4295,68 +4348,6 @@ virNetworkObjUpdate(virNetworkObjPtr network,
     return ret;
 }
 
-/*
- * virNetworkObjIsDuplicate:
- * @nets : virNetworkObjListPtr to search
- * @def  : virNetworkDefPtr definition of network to lookup
- * @check_active: If true, ensure that network is not active
- *
- * Returns: -1 on error
- *          0 if network is new
- *          1 if network is a duplicate
- */
-int
-virNetworkObjIsDuplicate(virNetworkObjListPtr nets,
-                         virNetworkDefPtr def,
-                         bool check_active)
-{
-    int ret = -1;
-    virNetworkObjPtr net = NULL;
-
-    /* See if a network with matching UUID already exists */
-    net = virNetworkObjFindByUUID(nets, def->uuid);
-    if (net) {
-        /* UUID matches, but if names don't match, refuse it */
-        if (STRNEQ(net->def->name, def->name)) {
-            char uuidstr[VIR_UUID_STRING_BUFLEN];
-            virUUIDFormat(net->def->uuid, uuidstr);
-            virReportError(VIR_ERR_OPERATION_FAILED,
-                           _("network '%s' is already defined with uuid %s"),
-                           net->def->name, uuidstr);
-            goto cleanup;
-        }
-
-        if (check_active) {
-            /* UUID & name match, but if network is already active, refuse it */
-            if (virNetworkObjIsActive(net)) {
-                virReportError(VIR_ERR_OPERATION_INVALID,
-                               _("network is already active as '%s'"),
-                               net->def->name);
-                goto cleanup;
-            }
-        }
-
-        ret = 1;
-    } else {
-        /* UUID does not match, but if a name matches, refuse it */
-        net = virNetworkObjFindByName(nets, def->name);
-        if (net) {
-            char uuidstr[VIR_UUID_STRING_BUFLEN];
-            virUUIDFormat(net->def->uuid, uuidstr);
-            virReportError(VIR_ERR_OPERATION_FAILED,
-                           _("network '%s' already exists with uuid %s"),
-                           def->name, uuidstr);
-            goto cleanup;
-        }
-        ret = 0;
-    }
-
- cleanup:
-    virNetworkObjEndAPI(&net);
-    return ret;
-}
-
-
 #define MATCH(FLAG) (flags & (FLAG))
 static bool
 virNetworkMatch(virNetworkObjPtr netobj,

src/conf/network_conf.h

@@ -316,9 +316,13 @@ void virNetworkDefFree(virNetworkDefPtr def);
 typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
                                         virNetworkDefPtr def);
 
+enum {
+    VIR_NETWORK_OBJ_LIST_ADD_LIVE = (1 << 0),
+    VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
+};
 virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
                                      virNetworkDefPtr def,
-                                     bool live);
+                                     unsigned int flags);
 void virNetworkObjAssignDef(virNetworkObjPtr network,
                             virNetworkDefPtr def,
                             bool live);
@@ -414,10 +418,6 @@ virNetworkObjUpdate(virNetworkObjPtr obj,
                     const char *xml,
                     unsigned int flags);  /* virNetworkUpdateFlags */
 
-int virNetworkObjIsDuplicate(virNetworkObjListPtr nets,
-                             virNetworkDefPtr def,
-                             bool check_active);
-
 VIR_ENUM_DECL(virNetworkForward)
 
 # define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE   \

src/libvirt_private.syms

@@ -573,7 +573,6 @@ virNetworkObjFindByNameLocked;
 virNetworkObjFindByUUID;
 virNetworkObjFindByUUIDLocked;
 virNetworkObjGetPersistentDef;
-virNetworkObjIsDuplicate;
 virNetworkObjListExport;
 virNetworkObjListForEach;
 virNetworkObjListGetNames;

src/network/bridge_driver.c

@@ -2748,8 +2748,7 @@ static int networkIsPersistent(virNetworkPtr net)
 
 static int
 networkValidate(virNetworkDriverStatePtr driver,
-                virNetworkDefPtr def,
-                bool check_active)
+                virNetworkDefPtr def)
 {
     size_t i, j;
     bool vlanUsed, vlanAllowed, badVlanUse = false;
@@ -2759,10 +2758,6 @@ networkValidate(virNetworkDriverStatePtr driver,
     bool bandwidthAllowed = true;
     bool usesInterface = false, usesAddress = false;
 
-    /* check for duplicate networks */
-    if (virNetworkObjIsDuplicate(driver->networks, def, check_active) < 0)
-        return -1;
-
     /* Only the three L3 network types that are configured by libvirt
      * need to have a bridge device name / mac address provided
      */
@@ -2980,14 +2975,16 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
     if (virNetworkCreateXMLEnsureACL(conn, def) < 0)
         goto cleanup;
 
-    if (networkValidate(driver, def, true) < 0)
+    if (networkValidate(driver, def) < 0)
         goto cleanup;
 
     /* NB: even though this transient network hasn't yet been started,
      * we assign the def with live = true in anticipation that it will
      * be started momentarily.
      */
-    if (!(network = virNetworkAssignDef(driver->networks, def, true)))
+    if (!(network = virNetworkAssignDef(driver->networks, def,
+                                        VIR_NETWORK_OBJ_LIST_ADD_LIVE |
+                                        VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE)))
         goto cleanup;
     def = NULL;
 
@@ -3028,10 +3025,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
     if (virNetworkDefineXMLEnsureACL(conn, def) < 0)
         goto cleanup;
 
-    if (networkValidate(driver, def, false) < 0)
+    if (networkValidate(driver, def) < 0)
         goto cleanup;
 
-    if (!(network = virNetworkAssignDef(driver->networks, def, false)))
+    if (!(network = virNetworkAssignDef(driver->networks, def, 0)))
         goto cleanup;
 
     /* def was assigned to network object */

src/parallels/parallels_network.c

@@ -239,7 +239,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
         goto cleanup;
     }
 
-    if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
+    if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
         goto cleanup;
     def = NULL;
     net->active = 1;
@@ -273,7 +273,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
     }
     def->uuid_specified = 1;
 
-    if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
+    if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
         goto cleanup;
 
     net->active = 1;

src/test/test_driver.c

@@ -782,7 +782,7 @@ testOpenDefault(virConnectPtr conn)
 
     if (!(netdef = virNetworkDefParseString(defaultNetworkXML)))
         goto error;
-    if (!(netobj = virNetworkAssignDef(privconn->networks, netdef, false))) {
+    if (!(netobj = virNetworkAssignDef(privconn->networks, netdef, 0))) {
         virNetworkDefFree(netdef);
         goto error;
     }
@@ -1149,7 +1149,7 @@ testParseNetworks(testConnPtr privconn,
         if (!def)
             goto error;
 
-        if (!(obj = virNetworkAssignDef(privconn->networks, def, false))) {
+        if (!(obj = virNetworkAssignDef(privconn->networks, def, 0))) {
             virNetworkDefFree(def);
             goto error;
         }
@@ -3627,7 +3627,9 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
     if ((def = virNetworkDefParseString(xml)) == NULL)
         goto cleanup;
 
-    if (!(net = virNetworkAssignDef(privconn->networks, def, true)))
+    if (!(net = virNetworkAssignDef(privconn->networks, def,
+                                    VIR_NETWORK_OBJ_LIST_ADD_LIVE |
+                                    VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE)))
         goto cleanup;
     def = NULL;
     net->active = 1;
@@ -3658,7 +3660,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
     if ((def = virNetworkDefParseString(xml)) == NULL)
         goto cleanup;
 
-    if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
+    if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
         goto cleanup;
     def = NULL;