On 05/30/2017 08:13 AM, Bjoern Walk wrote:
> John Ferlan <jferlan@xxxxxxxxxx> [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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>
> Otherwise, looks good to me.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
