Re: [PATCH v2 18/24] virNetworkObjFindBy*: Return an reference to found object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>> ---
>>  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);
 }

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]