5:53 a.m.
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
5:53 a.m.
New subject: [libvirt] [PATCH v4 1/3] interface: Introduce virInterfaceObjNew
Create/use a helper to perform the object allocation
Signed-off-by: John Ferlan <jferlan(a)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
9:31 a.m.
New subject: [libvirt] [PATCH v4 1/3] interface: Introduce virInterfaceObjNew
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(a)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
5:53 a.m.
New subject: [libvirt] [PATCH v4 2/3] interface: Consume @def in virInterfaceObjNew
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(a)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
10:20 a.m.
New subject: [libvirt] [PATCH v4 2/3] interface: Consume @def in virInterfaceObjNew
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(a)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
5:53 a.m.
New subject: [libvirt] [PATCH v4 3/3] interface: Convert virInterfaceObj to use virObjectLockable
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(a)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
10:23 a.m.
New subject: [libvirt] [PATCH v4 3/3] interface: Convert virInterfaceObj to use virObjectLockable
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(a)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
2727
days inactive
2729
days old
6 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan