On Thu, Mar 05, 2015 at 12:05:20 +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 | 49 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 42 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d3f3f4a..529ba2b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -129,9 +129,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); @@ -264,6 +262,11 @@ networkRemoveInactive(virNetworkObjPtr net)
int ret = -1;
+ virObjectRef(net);
Since you've already added proper reference counting in the previous patch and the callers of this function will always have the reference you don't need to ref it here ...
+ virObjectUnlock(net); + networkDriverLock(); + virObjectLock(net);
... also since there are only two functions that call tis piece of code you can avoid removing the network lock from them and avoid doing this locking dance.
+ /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) {
...
@@ -700,11 +705,9 @@ networkStateAutoStart(void) if (!driver) return;
- networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig,
In the callback above the networkStartNetwork() function is called that accesses the network driver via the reference in the global variable, but the accessed member is driver->stateDir which is immutable when looking at the code. Please annotate (in a separate patch) that the following fields are immutable: driver->networkConfigDir driver->networkAutostartDir driver->stateDir driver->pidDir driver->dnsmasqStateDir driver->radvdStateDir and that the network list is self locking so that people after me won't need to find it out again. ACK Peter