6:40 a.m.
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(a)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