On Fri, Mar 06, 2015 at 11:12:21 +0100, Michal Privoznik wrote:
On 06.03.2015 10:23, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 12:05:19 +0100, Michal Privoznik wrote:
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++-- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7af303e..3d318ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -164,6 +165,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -194,6 +196,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -491,13 +494,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network;
error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL;
} diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8501603..d3f3f4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; }
@@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network);
As virNetworkRemoveInactive() now doesn't free the object due to the existing reference, but unlocks it ...
- network = NULL;
... and you don't invalidate the pointer, you'd touch the unlocked object in virNetworkObjEndAPI() and try to unlock it once more.
goto cleanup; } /* if network was active already, just undo new persistent
NACK, virNetworkRemoveInactive needs to be fixed first.
Correct. Nice catch! Would I make you reconsider NACK if I squash this into this commit?
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3d318ce..20a257b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -668,17 +668,18 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
+ /* It's not needed to reference the @net as we are called + * from an API which surely already has at least one + * reference to the object. */ virObjectUnlock(net); virObjectLock(nets); + virObjectLock(net); for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - virObjectUnlock(net); virObjectUnref(net); break; } - virObjectUnlock(nets->objs[i]); } virObjectUnlock(nets); }
Fair enough. ACK with the above fixup. Peter