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

Re: [libvirt] [PATCH v3 9/9] interface: Convert virInterfaceObj to use virObjectLockable




On 05/30/2017 08:13 AM, Bjoern Walk wrote:
> John Ferlan <jferlan redhat com> [2017-05-30, 06:43AM -0400]:
>> [...]
>> void
>> -virInterfaceObjFree(virInterfaceObjPtr obj)
>> +virInterfaceObjEndAPI(virInterfaceObjPtr *obj)
> 
> Naming is hard, and I don't have any better suggestion. Just wanted to
> say that the name is, maybe, improvable :)
> 

It's a common nomenclature for libvirt... virDomainObjEndAPI,
virNetworkObjEndAPI, and virSecretObjEndAPI.

>> {
>> -    if (!obj)
>> +    if (!*obj)
>>         return;
>>
>> -    virInterfaceDefFree(obj->def);
>> -    virMutexDestroy(&obj->lock);
>> -    VIR_FREE(obj);
>> +    virObjectUnlock(*obj);
>> +    virObjectUnref(*obj);
>> +    *obj = NULL;
> 
> I'm a bit conflicted if this is strictly necessary. In what situation
> would this bite a consumer if we don't NULL obj? At least in this patch
> I can't find any.
> 

Although perhaps true - it's the common way this happens for other
vir*ObjEndAPI source code. Since it's theoretically possible an Unref
could cause the object to be Disposed (if refcnt == 0), setting *obj =
NULL at least "ensures" the caller misusing the object would be a NULL
deref rather than referencing something it no longer owned.

>> }
>>
>>
>> @@ -140,18 +150,18 @@
>> virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
>>         virInterfaceObjPtr obj = interfaces->objs[i];
>>         virInterfaceDefPtr def;
>>
>> -        virInterfaceObjLock(obj);
>> +        virObjectLock(obj);
>>         def = obj->def;
>>         if (STRCASEEQ(def->mac, mac)) {
>>             if (matchct < maxmatches) {
>>                 if (VIR_STRDUP(matches[matchct], def->name) < 0) {
>> -                    virInterfaceObjUnlock(obj);
>> +                    virObjectUnlock(obj);
>>                     goto error;
>>                 }
>>                 matchct++;
>>             }
>>         }
>> -        virInterfaceObjUnlock(obj);
>> +        virObjectUnlock(obj);
>>     }
>>     return matchct;
>>
>> @@ -173,11 +183,11 @@
>> virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces,
>>         virInterfaceObjPtr obj = interfaces->objs[i];
>>         virInterfaceDefPtr def;
>>
>> -        virInterfaceObjLock(obj);
>> +        virObjectLock(obj);
>>         def = obj->def;
>>         if (STREQ(def->name, name))
>> -            return obj;
>> -        virInterfaceObjUnlock(obj);
>> +            return virObjectRef(obj);
>> +        virObjectUnlock(obj);
>>     }
>>
>>     return NULL;
>> @@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr
>> interfaces)
>>     size_t i;
>>
>>     for (i = 0; i < interfaces->count; i++)
>> -        virInterfaceObjFree(interfaces->objs[i]);
>> +        virObjectUnref(interfaces->objs[i]);
>>     VIR_FREE(interfaces->objs);
>>     VIR_FREE(interfaces);
>> }
>> @@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr
>> interfaces)
>>         VIR_FREE(xml);
>>         if (!(obj = virInterfaceObjListAssignDef(dest, backup)))
>>             goto error;
>> -        virInterfaceObjUnlock(obj); /* locked by
>> virInterfaceObjListAssignDef */
>> +        virInterfaceObjEndAPI(&obj);
>>     }
>>
>>     return dest;
>> @@ -256,13 +266,12 @@
>> virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
>>
>>     if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
>>                                 interfaces->count, obj) < 0) {
>> -        virInterfaceObjUnlock(obj);
>> -        virInterfaceObjFree(obj);
>> +        virInterfaceObjEndAPI(&obj);
>>         return NULL;
>>     }
>>
>>     obj->def = def;
>> -    return obj;
>> +    return virObjectRef(obj);
>>
>> }
>>
>> @@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr
>> interfaces,
>> {
>>     size_t i;
>>
>> -    virInterfaceObjUnlock(obj);
>> +    virObjectUnlock(obj);
>>     for (i = 0; i < interfaces->count; i++) {
>> -        virInterfaceObjLock(interfaces->objs[i]);
>> +        virObjectLock(interfaces->objs[i]);
>>         if (interfaces->objs[i] == obj) {
>> -            virInterfaceObjUnlock(interfaces->objs[i]);
>> -            virInterfaceObjFree(interfaces->objs[i]);
>> +            virObjectUnlock(interfaces->objs[i]);
>> +            virObjectUnref(interfaces->objs[i]);
> 
> Here you could use virInterfaceObjEndAPI if the nulling would be
> dropped. Small advantage, I know.

Understood, I suppose this could also have taken the
"virInterfaceObjEndAPI(&obj);" option...

Still eventually this is changing from a forward linked list removal to
a removal from a hash table.

For the sake of my local branches, I'd prefer to keep as is, although if
there's a strong desire for something different I'm not opposed to
adjusting.

Thanks for taking a look at the last two!

John

>>
>>             VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count);
>>             break;
>>         }
>> -        virInterfaceObjUnlock(interfaces->objs[i]);
>> +        virObjectUnlock(interfaces->objs[i]);
>>     }
>> }
>>
>> @@ -297,10 +306,10 @@
>> virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces,
>>
>>     for (i = 0; (i < interfaces->count); i++) {
>>         virInterfaceObjPtr obj = interfaces->objs[i];
>> -        virInterfaceObjLock(obj);
>> +        virObjectLock(obj);
>>         if (wantActive == virInterfaceObjIsActive(obj))
>>             ninterfaces++;
>> -        virInterfaceObjUnlock(obj);
>> +        virObjectUnlock(obj);
>>     }
>>
>>     return ninterfaces;
>> @@ -320,16 +329,16 @@
>> virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces,
>>         virInterfaceObjPtr obj = interfaces->objs[i];
>>         virInterfaceDefPtr def;
>>
>> -        virInterfaceObjLock(obj);
>> +        virObjectLock(obj);
>>         def = obj->def;
>>         if (wantActive == virInterfaceObjIsActive(obj)) {
>>             if (VIR_STRDUP(names[nnames], def->name) < 0) {
>> -                virInterfaceObjUnlock(obj);
>> +                virObjectUnlock(obj);
>>                 goto failure;
>>             }
>>             nnames++;
>>         }
>> -        virInterfaceObjUnlock(obj);
>> +        virObjectUnlock(obj);
>>     }
>>
>>     return nnames;
>> diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
>> index 3934e63..2b9e1b2 100644
>> --- a/src/conf/virinterfaceobj.h
>> +++ b/src/conf/virinterfaceobj.h
>> @@ -28,6 +28,9 @@ typedef virInterfaceObj *virInterfaceObjPtr;
>> typedef struct _virInterfaceObjList virInterfaceObjList;
>> typedef virInterfaceObjList *virInterfaceObjListPtr;
>>
>> +void
>> +virInterfaceObjEndAPI(virInterfaceObjPtr *obj);
>> +
>> virInterfaceDefPtr
>> virInterfaceObjGetDef(virInterfaceObjPtr obj);
>>
>> @@ -68,12 +71,6 @@ void
>> virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
>>                           virInterfaceObjPtr obj);
>>
>> -void
>> -virInterfaceObjLock(virInterfaceObjPtr obj);
>> -
>> -void
>> -virInterfaceObjUnlock(virInterfaceObjPtr obj);
>> -
>> typedef bool
>> (*virInterfaceObjListFilter)(virConnectPtr conn,
>>                              virInterfaceDefPtr def);
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 9be50cb..5d6cb5e 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -910,6 +910,7 @@ virDomainObjListRename;
>>
>>
>> # conf/virinterfaceobj.h
>> +virInterfaceObjEndAPI;
>> virInterfaceObjGetDef;
>> virInterfaceObjIsActive;
>> virInterfaceObjListAssignDef;
>> @@ -921,9 +922,7 @@ virInterfaceObjListGetNames;
>> virInterfaceObjListNew;
>> virInterfaceObjListNumOfInterfaces;
>> virInterfaceObjListRemove;
>> -virInterfaceObjLock;
>> virInterfaceObjSetActive;
>> -virInterfaceObjUnlock;
>>
>>
>> # conf/virnetworkobj.h
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 511d65f..9a1c8a5 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -1027,7 +1027,7 @@ testParseInterfaces(testDriverPtr privconn,
>>         }
>>
>>         virInterfaceObjSetActive(obj, true);
>> -        virInterfaceObjUnlock(obj);
>> +        virInterfaceObjEndAPI(&obj);
>>     }
>>
>>     ret = 0;
>> @@ -3718,7 +3718,7 @@ testInterfaceLookupByName(virConnectPtr conn,
>>
>>     ret = virGetInterface(conn, def->name, def->mac);
>>
>> -    virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     return ret;
>> }
>>
>> @@ -3769,7 +3769,7 @@ testInterfaceIsActive(virInterfacePtr iface)
>>
>>     ret = virInterfaceObjIsActive(obj);
>>
>> -    virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     return ret;
>> }
>>
>> @@ -3881,7 +3881,7 @@ testInterfaceGetXMLDesc(virInterfacePtr iface,
>>
>>     ret = virInterfaceDefFormat(def);
>>
>> -    virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     return ret;
>> }
>>
>> @@ -3912,8 +3912,7 @@ testInterfaceDefineXML(virConnectPtr conn,
>>
>>  cleanup:
>>     virInterfaceDefFree(def);
>> -    if (obj)
>> -        virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     testDriverUnlock(privconn);
>>     return ret;
>> }
>> @@ -3956,7 +3955,7 @@ testInterfaceCreate(virInterfacePtr iface,
>>     ret = 0;
>>
>>  cleanup:
>> -    virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     return ret;
>> }
>>
>> @@ -3983,7 +3982,7 @@ testInterfaceDestroy(virInterfacePtr iface,
>>     ret = 0;
>>
>>  cleanup:
>> -    virInterfaceObjUnlock(obj);
>> +    virInterfaceObjEndAPI(&obj);
>>     return ret;
>> }
>>
>> -- 
>> 2.9.4
>>
>> -- 
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 
> Otherwise, looks good to me.
> 


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