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 <[email protected]> > --- > 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
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
