There's no need to lock the network driver, as network driver initialization is done prior accepting any client. There's nobody to hop in and do something over partially initialized driver. Nor qemu driver is doing that. ==30532== Observed (incorrect) order is: acquisition of lock at 0x1439EF50 ==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0x5324895: virMutexLock (virthread.c:88) ==30532== by 0x5307E86: virObjectLock (virobject.c:323) ==30532== by 0x5396440: virNetworkObjListForEach (network_conf.c:4511) ==30532== by 0x19B29308: networkStateInitialize (bridge_driver.c:686) ==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777) ==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906) ==30532== by 0x5324B6A: virThreadHelper (virthread.c:197) ==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so) ==30532== by 0xA4EDC8C: clone (in /lib64/libc-2.19.so) ==30532== ==30532== followed by a later acquisition of lock at 0x1439CD60 ==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0x5324895: virMutexLock (virthread.c:88) ==30532== by 0x19B27B2C: networkDriverLock (bridge_driver.c:102) ==30532== by 0x19B27B60: networkGetDnsmasqCaps (bridge_driver.c:113) ==30532== by 0x19B2856A: networkUpdateState (bridge_driver.c:389) ==30532== by 0x53963E9: virNetworkObjListForEachHelper (network_conf.c:4488) ==30532== by 0x52E2224: virHashForEach (virhash.c:521) ==30532== by 0x539645B: virNetworkObjListForEach (network_conf.c:4512) ==30532== by 0x19B29308: networkStateInitialize (bridge_driver.c:686) ==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777) ==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906) ==30532== by 0x5324B6A: virThreadHelper (virthread.c:197) ==30532== ==30532== Required order was established by acquisition of lock at 0x1439CD60 ==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0x5324895: virMutexLock (virthread.c:88) ==30532== by 0x19B27B2C: networkDriverLock (bridge_driver.c:102) ==30532== by 0x19B28DF9: networkStateInitialize (bridge_driver.c:609) ==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777) ==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906) ==30532== by 0x5324B6A: virThreadHelper (virthread.c:197) ==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so) ==30532== by 0xA4EDC8C: clone (in /lib64/libc-2.19.so) ==30532== ==30532== followed by a later acquisition of lock at 0x1439EF50 ==30532== at 0x4C31A26: pthread_mutex_lock (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0x5324895: virMutexLock (virthread.c:88) ==30532== by 0x5307E86: virObjectLock (virobject.c:323) ==30532== by 0x538A09C: virNetworkAssignDef (network_conf.c:527) ==30532== by 0x5391EB2: virNetworkLoadState (network_conf.c:3008) ==30532== by 0x53922D4: virNetworkLoadAllState (network_conf.c:3128) ==30532== by 0x19B2929A: networkStateInitialize (bridge_driver.c:671) ==30532== by 0x53E1CCC: virStateInitialize (libvirt.c:777) ==30532== by 0x11DEB7: daemonRunStateInit (libvirtd.c:906) ==30532== by 0x5324B6A: virThreadHelper (virthread.c:197) ==30532== by 0x4C30456: ??? (in /usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so) ==30532== by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so) Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a007388..b3dcadc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -606,7 +606,6 @@ networkStateInitialize(bool privileged, VIR_FREE(network_driver); goto error; } - networkDriverLock(network_driver); /* configuration/state paths are one of * ~/.config/libvirt/... (session/unprivileged) @@ -677,8 +676,6 @@ networkStateInitialize(bool privileged, network_driver->networkAutostartDir) < 0) goto error; - networkDriverUnlock(network_driver); - /* Update the internal status of all allegedly active * networks according to external conditions on the host * (i.e. anything that isn't stored directly in each -- 2.0.5

Lets not give a bad example and check for return values of virNetwork* APIs called within the test. Even though it's unlikely that any API will fail, it can happen. We're connected to the test driver after all, and our API sequence is correct. So test driver should fail only in case of bug or OOM. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/objecteventtest.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 052dbe5..95fbfec 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -417,7 +417,7 @@ testNetworkCreateXML(const void *data) &counter, NULL); net = virNetworkCreateXML(test->conn, networkDef); - if (virEventRunDefaultImpl() < 0) { + if (!net || virEventRunDefaultImpl() < 0) { ret = -1; goto cleanup; } @@ -429,10 +429,10 @@ testNetworkCreateXML(const void *data) cleanup: virConnectNetworkEventDeregisterAny(test->conn, id); - virNetworkDestroy(net); - - virNetworkFree(net); - + if (net) { + virNetworkDestroy(net); + virNetworkFree(net); + } return ret; } @@ -455,7 +455,7 @@ testNetworkDefine(const void *data) /* Make sure the define event is triggered */ net = virNetworkDefineXML(test->conn, networkDef); - if (virEventRunDefaultImpl() < 0) { + if (!net || virEventRunDefaultImpl() < 0) { ret = -1; goto cleanup; } @@ -481,7 +481,8 @@ testNetworkDefine(const void *data) cleanup: virConnectNetworkEventDeregisterAny(test->conn, id); - virNetworkFree(net); + if (net) + virNetworkFree(net); return ret; } @@ -494,6 +495,9 @@ testNetworkStartStopEvent(const void *data) int id; int ret = 0; + if (!test->net) + return -1; + lifecycleEventCounter_reset(&counter); id = virConnectNetworkEventRegisterAny(test->conn, test->net, @@ -509,7 +513,7 @@ testNetworkStartStopEvent(const void *data) } if (counter.startEvents != 1 || counter.stopEvents != 1 || - counter.unexpectedEvents > 0) { + counter.unexpectedEvents > 0) { ret = -1; goto cleanup; } @@ -567,13 +571,16 @@ mymain(void) ret = EXIT_FAILURE; /* Define a test network */ - test.net = virNetworkDefineXML(test.conn, networkDef); + if (!(test.net = virNetworkDefineXML(test.conn, networkDef))) + ret = EXIT_FAILURE; if (virtTestRun("Network start stop events ", testNetworkStartStopEvent, &test) < 0) ret = EXIT_FAILURE; /* Cleanup */ - virNetworkUndefine(test.net); - virNetworkFree(test.net); + if (test.net) { + virNetworkUndefine(test.net); + virNetworkFree(test.net); + } virConnectClose(test.conn); virEventRemoveTimeout(timer); -- 2.0.5

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&gt; --- 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

On 20.03.2015 20:59, John Ferlan wrote: Thanks. I've kept the old names to keep consistency with domain_conf.c counterpart. It's gonna be easier later, when we will merge vir{Domain,Network,...}ObjList into virObjectList. Michal