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.