[libvirt] [PATCH v4 0/3] Multiple cleanups within interfaceobj and interface driver
v3: https://www.redhat.com/archives/libvir-list/2017-May/msg01161.html NB: The first 7 patches from v3 were pushed since they were ACK'd. Difference in v4: Patch 1 was essentially ACK'd, but I chose to hold onto it to show the context Patch 2 is the new patch/context - move the @def processing into ObjNew Patch 3 is mostly unchanged; however, I realized during processing of the other code that with the model chosen in virInterfaceObjListAssignDef to return an object that has had both the @lock and @ref incremented so that callers can use the EndAPI to effectively Unlock and Unref that means when calling virInterfaceObjListRemove all that's done is a single Unlock/Unref leaving the object with still 1 reference (and undeleted). That means the caller (testInterfaceUndefine) must make that Unref call of the object. This is no different than how Domain's work, although it's a bit more obtuse there because callers of domain add will make that "second" reference to the object and my goal in this is to be a bit more obvious... The easier way to think about this is for objects only in one list, when leaving the Add/Assign API's, there are 2 Refs and 1 Lock on the object. For objects in 2 lists, there are 3 Refs and 1 Lock. Each caller always calls EndAPI dropping a Ref and the Lock. When it's time to Remove the object, a Find is done - which increments a Ref and Lock and then the Remove API called. The Remove API will effectively Unref for the object in a list (once or twice) and return the Unlocked object w/ 1 Ref. John Ferlan (3): interface: Introduce virInterfaceObjNew interface: Consume @def in virInterfaceObjNew interface: Convert virInterfaceObj to use virObjectLockable po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 108 +++++++++++++++++++++++++++------------------ src/conf/virinterfaceobj.h | 9 ++-- src/libvirt_private.syms | 3 +- src/test/test_driver.c | 16 +++---- 5 files changed, 77 insertions(+), 60 deletions(-) -- 2.9.4
Create/use a helper to perform the object allocation Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 8bd8094..1e3f25c 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -46,6 +46,27 @@ struct _virInterfaceObjList { /* virInterfaceObj manipulation */ +static virInterfaceObjPtr +virInterfaceObjNew(void) +{ + virInterfaceObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + + virInterfaceObjLock(obj); + + return obj; +} + + void virInterfaceObjLock(virInterfaceObjPtr obj) { @@ -230,18 +251,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return obj; } - if (VIR_ALLOC(obj) < 0) - return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virInterfaceObjNew())) return NULL; - } - virInterfaceObjLock(obj); if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { + virInterfaceObjUnlock(obj); virInterfaceObjFree(obj); return NULL; } -- 2.9.4
On Sat, Jun 03, 2017 at 06:53:21AM -0400, John Ferlan wrote:
Create/use a helper to perform the object allocation
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 8bd8094..1e3f25c 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -46,6 +46,27 @@ struct _virInterfaceObjList {
/* virInterfaceObj manipulation */
+static virInterfaceObjPtr +virInterfaceObjNew(void) +{ + virInterfaceObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + + virInterfaceObjLock(obj);
Nothing in the function name suggests that the returned object is going to be locked already, but we do the same thing for domains, secrets, and NWFilter, so let's just say I'm okay with such design. ACK. Erik
Move the consumption of @def in virInterfaceObjNew and then handle that in the error path of virInterfaceObjListAssignDef since it's caller expects to need to free @def when NULL is returned and so would virInterfaceObjFree. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1e3f25c..159fcb2 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -47,7 +47,7 @@ struct _virInterfaceObjList { /* virInterfaceObj manipulation */ static virInterfaceObjPtr -virInterfaceObjNew(void) +virInterfaceObjNew(virInterfaceDefPtr def) { virInterfaceObjPtr obj; @@ -62,6 +62,7 @@ virInterfaceObjNew(void) } virInterfaceObjLock(obj); + obj->def = def; return obj; } @@ -251,17 +252,17 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return obj; } - if (!(obj = virInterfaceObjNew())) + if (!(obj = virInterfaceObjNew(def))) return NULL; if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { + obj->def = NULL; virInterfaceObjUnlock(obj); virInterfaceObjFree(obj); return NULL; } - obj->def = def; return obj; } -- 2.9.4
On Sat, Jun 03, 2017 at 06:53:22AM -0400, John Ferlan wrote:
Move the consumption of @def in virInterfaceObjNew and then handle that in the error path of virInterfaceObjListAssignDef since it's caller expects to need to free @def when NULL is returned and so would virInterfaceObjFree.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1e3f25c..159fcb2 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -47,7 +47,7 @@ struct _virInterfaceObjList { /* virInterfaceObj manipulation */
static virInterfaceObjPtr -virInterfaceObjNew(void) +virInterfaceObjNew(virInterfaceDefPtr def) { virInterfaceObjPtr obj;
@@ -62,6 +62,7 @@ virInterfaceObjNew(void) }
virInterfaceObjLock(obj); + obj->def = def;
I'd probably move this before locking the object, first initializing it and then locking and returning it, but that's a matter of preference. ACK. Erik
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock. This commit also introduces virInterfaceObjEndAPI in order to handle the lock unlock and object unref in one call for consumers returning a NULL obj upon return. This removes the need for virInterfaceObj{Lock|Unlock} external API's. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 106 ++++++++++++++++++++++++--------------------- src/conf/virinterfaceobj.h | 9 ++-- src/libvirt_private.syms | 3 +- src/test/test_driver.c | 16 +++---- 5 files changed, 68 insertions(+), 67 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 142b4d3..923d647 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -44,7 +44,6 @@ src/conf/storage_adapter_conf.c src/conf/storage_conf.c src/conf/virchrdev.c src/conf/virdomainobjlist.c -src/conf/virinterfaceobj.c src/conf/virnetworkobj.c src/conf/virnodedeviceobj.c src/conf/virnwfilterobj.c diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 159fcb2..106f232 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { - virMutex lock; + virObjectLockable parent; bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ @@ -46,22 +46,45 @@ struct _virInterfaceObjList { /* virInterfaceObj manipulation */ +static virClassPtr virInterfaceObjClass; +static void virInterfaceObjDispose(void *obj); + +static int +virInterfaceObjOnceInit(void) +{ + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + "virInterfaceObj", + sizeof(virInterfaceObj), + virInterfaceObjDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virInterfaceObj) + + +static void +virInterfaceObjDispose(void *opaque) +{ + virInterfaceObjPtr obj = opaque; + + virInterfaceDefFree(obj->def); +} + + static virInterfaceObjPtr virInterfaceObjNew(virInterfaceDefPtr def) { virInterfaceObjPtr obj; - if (VIR_ALLOC(obj) < 0) + if (virInterfaceObjInitialize() < 0) return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virObjectLockableNew(virInterfaceObjClass))) return NULL; - } - virInterfaceObjLock(obj); + virObjectLock(obj); obj->def = def; return obj; @@ -69,28 +92,14 @@ virInterfaceObjNew(virInterfaceDefPtr def) void -virInterfaceObjLock(virInterfaceObjPtr obj) +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) { - virMutexLock(&obj->lock); -} - - -void -virInterfaceObjUnlock(virInterfaceObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - - -void -virInterfaceObjFree(virInterfaceObjPtr obj) -{ - if (!obj) + if (!*obj) return; - virInterfaceDefFree(obj->def); - virMutexDestroy(&obj->lock); - VIR_FREE(obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; } @@ -141,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; @@ -174,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; @@ -191,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); } @@ -228,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; @@ -258,13 +267,10 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { obj->def = NULL; - virInterfaceObjUnlock(obj); - virInterfaceObjFree(obj); + virInterfaceObjEndAPI(&obj); return NULL; } - - return obj; - + return virObjectRef(obj); } @@ -274,17 +280,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]); VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } } @@ -298,10 +304,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; @@ -321,16 +327,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..70cd15e 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; } @@ -3929,6 +3928,7 @@ testInterfaceUndefine(virInterfacePtr iface) return -1; virInterfaceObjListRemove(privconn->ifaces, obj); + virObjectUnref(obj); return 0; } @@ -3956,7 +3956,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3983,7 +3983,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } -- 2.9.4
On Sat, Jun 03, 2017 at 06:53:23AM -0400, John Ferlan wrote:
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock.
This commit also introduces virInterfaceObjEndAPI in order to handle the lock unlock and object unref in one call for consumers returning a NULL obj upon return. This removes the need for virInterfaceObj{Lock|Unlock} external API's.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 106 ++++++++++++++++++++++++--------------------- src/conf/virinterfaceobj.h | 9 ++-- src/libvirt_private.syms | 3 +- src/test/test_driver.c | 16 +++---- 5 files changed, 68 insertions(+), 67 deletions(-)
ACK. Erik
participants (2)
-
Erik Skultety -
John Ferlan