11:42 a.m.
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: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/network_conf.c | 171 ++++++++++++++++++--------------------
src/conf/network_conf.h | 10 +--
src/libvirt_private.syms | 1 -
src/network/bridge_driver.c | 17 ++--
src/parallels/parallels_network.c | 4 +-
src/test/test_driver.c | 10 ++-
6 files changed, 99 insertions(+), 114 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d7c5dec..1fb06ef 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -504,6 +504,81 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
}
/*
+ * If flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE then this will
+ * refuse updating an existing def if the current def is Live
+ *
+ * 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
+ */
+static virNetworkObjPtr
+virNetworkAssignDefLocked(virNetworkObjListPtr nets,
+ virNetworkDefPtr def,
+ unsigned int flags)
+{
+ virNetworkObjPtr network;
+ virNetworkObjPtr ret = NULL;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ /* See if a network with matching UUID already exists */
+ if ((network = virNetworkObjFindByUUIDLocked(nets, def->uuid))) {
+ virObjectLock(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 (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 cleanup;
+
+ network->def = def;
+ network->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE);
+ virObjectRef(network);
+ }
+
+ ret = network;
+ network = NULL;
+
+ cleanup:
+ virNetworkObjEndAPI(&network);
+ return ret;
+}
+
+/*
* virNetworkAssignDef:
* @nets: list of all networks
* @def: the new NetworkDef (will be consumed by this function iff successful)
@@ -519,40 +594,14 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
virNetworkObjPtr
virNetworkAssignDef(virNetworkObjListPtr nets,
virNetworkDefPtr def,
- bool live)
+ unsigned int flags)
{
virNetworkObjPtr network;
- char uuidstr[VIR_UUID_STRING_BUFLEN];
virObjectLock(nets);
- if ((network = virNetworkObjFindByNameLocked(nets, def->name))) {
- virObjectUnlock(nets);
- virObjectLock(network);
- virNetworkObjAssignDef(network, def, live);
- return network;
- }
-
- if (!(network = virNetworkObjNew())) {
- virObjectUnlock(nets);
- return NULL;
- }
- virObjectLock(network);
-
- virUUIDFormat(def->uuid, uuidstr);
- if (virHashAddEntry(nets->objs, uuidstr, network) < 0)
- goto error;
-
- network->def = def;
- network->persistent = !live;
- virObjectRef(network);
+ network = virNetworkAssignDefLocked(nets, def, flags);
virObjectUnlock(nets);
return network;
-
- error:
- virObjectUnlock(nets);
- virNetworkObjEndAPI(&network);
- return NULL;
-
}
/*
@@ -3005,7 +3054,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 +3131,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 +4344,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,
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index 3e926f7..f69d999 100644
--- a/src/conf/network_conf.h
+++ b/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 \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1fb42ac..02edb75 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -573,7 +573,6 @@ virNetworkObjFindByNameLocked;
virNetworkObjFindByUUID;
virNetworkObjFindByUUIDLocked;
virNetworkObjGetPersistentDef;
-virNetworkObjIsDuplicate;
virNetworkObjListExport;
virNetworkObjListForEach;
virNetworkObjListGetNames;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index b3dcadc..c3bc6fe 100644
--- a/src/network/bridge_driver.c
+++ b/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 */
diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
index 1d3b694..0711558 100644
--- a/src/parallels/parallels_network.c
+++ b/src/parallels/parallels_network.c
@@ -226,7 +226,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
goto cleanup;
}
- if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
+ if (!(net = virNetworkAssignDef(privconn->networks, def, 0)))
goto cleanup;
net->active = 1;
net->autostart = 1;
@@ -258,7 +258,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
}
def->uuid_specified = 1;
- if (!(net = virNetworkAssignDef(privconn->networks, def, false))) {
+ if (!(net = virNetworkAssignDef(privconn->networks, def, 0))) {
virNetworkDefFree(def);
goto cleanup;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 1c9d573..07cc032 100644
--- a/src/test/test_driver.c
+++ b/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;
--
2.0.5