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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list