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

John Ferlan <jferlan(a)redhat.com&gt; [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 :)

> { > - 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.

> } > > > @@ -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.

> > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Otherwise, looks good to me.