On Tue, Mar 10, 2015 at 17:45:18 +0100, Michal Privoznik wrote:
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 71 ------------------------------------ src/network/bridge_driver_platform.h | 2 - tests/objectlocking.ml | 2 - 3 files changed, 75 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5ef9910..c6957c3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -88,16 +88,6 @@ VIR_LOG_INIT("network.bridge_driver");
static virNetworkDriverStatePtr driver;
- -static void networkDriverLock(void) -{ - virMutexLock(&driver->lock); -} -static void networkDriverUnlock(void) -{ - virMutexUnlock(&driver->lock); -} - static int networkStateCleanup(void);
static int networkStartNetwork(virNetworkObjPtr network); @@ -129,9 +119,7 @@ networkObjFromNetwork(virNetworkPtr net) virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN];
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); - networkDriverUnlock();
if (!network) { virUUIDFormat(net->uuid, uuidstr); @@ -557,12 +545,6 @@ networkStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) goto error;
- if (virMutexInit(&driver->lock) < 0) { - VIR_FREE(driver); - goto error; - } - networkDriverLock(); - /* configuration/state paths are one of * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). @@ -648,8 +630,6 @@ networkStateInitialize(bool privileged,
driver->networkEventState = virObjectEventStateNew();
- networkDriverUnlock(); - #ifdef HAVE_FIREWALLD if (!(sysbus = virDBusGetSystemBus())) { virErrorPtr err = virGetLastError(); @@ -683,8 +663,6 @@ networkStateInitialize(bool privileged, return ret;
error: - if (driver) - networkDriverUnlock(); networkStateCleanup(); goto cleanup; } @@ -700,11 +678,9 @@ networkStateAutoStart(void) if (!driver) return;
- networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig,
Although it's not obvious as the @driver isn't passed explicitly this internally calls networkStartDhcpDaemon which accesses @driver->dnsmasqStateDir which isn't immutable. Having the access to @driver hidden by the access to a global variable is really sneaky. I'd prefer if we'd pass it explicitly in this case.
NULL); - networkDriverUnlock(); }
/** @@ -719,7 +695,6 @@ networkStateReload(void) if (!driver) return 0;
- networkDriverLock(); virNetworkLoadAllState(driver->networks, driver->stateDir); virNetworkLoadAllConfigs(driver->networks, @@ -730,7 +705,6 @@ networkStateReload(void) virNetworkObjListForEach(driver->networks, networkAutostartConfig,
same problem as above
NULL); - networkDriverUnlock(); return 0; }
@@ -746,8 +720,6 @@ networkStateCleanup(void) if (!driver) return -1;
- networkDriverLock(); - virObjectEventStateFree(driver->networkEventState);
/* free inactive networks */ @@ -762,9 +734,6 @@ networkStateCleanup(void)
virObjectUnref(driver->dnsmasqCaps);
- networkDriverUnlock(); - virMutexDestroy(&driver->lock); - VIR_FREE(driver);
return 0; @@ -2478,9 +2447,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL;
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, uuid); - networkDriverUnlock(); if (!network) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); @@ -2506,9 +2473,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL;
- networkDriverLock(); network = virNetworkObjFindByName(driver->networks, name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), name); @@ -2532,12 +2497,10 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); nactive = virNetworkObjListNumOfNetworks(driver->networks, true, virConnectNumOfNetworksCheckACL, conn); - networkDriverUnlock();
return nactive; } @@ -2548,12 +2511,10 @@ static int networkConnectListNetworks(virConnectPtr conn, char **const names, in if (virConnectListNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, true, names, nnames, virConnectListNetworksCheckACL, conn); - networkDriverUnlock();
return got; } @@ -2565,12 +2526,10 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); ninactive = virNetworkObjListNumOfNetworks(driver->networks, false, virConnectNumOfDefinedNetworksCheckACL, conn); - networkDriverUnlock();
return ninactive; } @@ -2581,12 +2540,10 @@ static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const na if (virConnectListDefinedNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, false, names, nnames, virConnectListDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return got; }
@@ -2602,11 +2559,9 @@ networkConnectListAllNetworks(virConnectPtr conn, if (virConnectListAllNetworksEnsureACL(conn) < 0) goto cleanup;
- networkDriverLock(); ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); - networkDriverUnlock();
cleanup: return ret; @@ -2917,8 +2872,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL;
- networkDriverLock();
This function will have the same problem as it starts the network.
- if (!(def = virNetworkDefParseString(xml))) goto cleanup;
@@ -2955,7 +2908,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -2967,8 +2919,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL;
- networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup;
@@ -3010,7 +2960,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3022,8 +2971,6 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL;
- networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3067,7 +3014,6 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3091,8 +3037,6 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1);
- networkDriverLock(); -
This API is restarting the DHCP daemon in some cases so it will share the problem above.
network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3238,7 +3182,6 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3248,7 +3191,6 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL;
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid);
here too
if (!network) { @@ -3272,7 +3214,6 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
The driver lock can't be dropped in that case. I think we can employ a similar mechanism as in qemu driver, where data that changes is stored in a reference counted object and the object pointer is replaced under a lock in case it's required to change the data. The rest of the driver lock dropping looks okay to me though. Peter