[libvirt] [PATCH v2 00/11] Multiple cleanups within interfaceobj and interface driver
v1: https://www.redhat.com/archives/libvir-list/2017-April/msg01225.html I've been informed via internal IRC that the patches 9, 10, and 11 don't apply cleanly - so here's a clean series. John John Ferlan (11): interface: Consistently use 'obj' for a virInterfaceObjPtr interface: Remove some unnecessary goto's for Interface tests interface: Use virInterfaceDefPtr rather than deref from virInterfaceObjPtr interface: Make _virInterfaceObj struct private interface: Make _virInterfaceObjList struct private interface: Rename some virInterfaceObj* API's interface: Clean up virInterfaceObjListFindByMACString interface: Introduce virInterfaceObjNew interface: Introduce virInterfaceObjEndAPI interface: Convert virInterfaceObj to use virObjectLockable interface: Alter virInterfaceObjListAssignDef @def param po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 277 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 72 ++++++------ src/libvirt_private.syms | 19 ++-- src/test/test_driver.c | 145 ++++++++++++------------ 5 files changed, 295 insertions(+), 219 deletions(-) -- 2.9.4
Alter variable names to be obj rather than 'iface' and/or 'obj'. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 48 ++++++++++++++-------------- src/conf/virinterfaceobj.h | 4 +-- src/test/test_driver.c | 78 +++++++++++++++++++++++----------------------- 3 files changed, 65 insertions(+), 65 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e80db23..62c3735 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -51,14 +51,14 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj) void -virInterfaceObjFree(virInterfaceObjPtr iface) +virInterfaceObjFree(virInterfaceObjPtr obj) { - if (!iface) + if (!obj) return; - virInterfaceDefFree(iface->def); - virMutexDestroy(&iface->lock); - VIR_FREE(iface); + virInterfaceDefFree(obj->def); + virMutexDestroy(&obj->lock); + VIR_FREE(obj); } @@ -136,7 +136,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr src, for (i = 0; i < cnt; i++) { virInterfaceDefPtr def = src->objs[i]->def; virInterfaceDefPtr backup; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; char *xml = virInterfaceDefFormat(def); if (!xml) @@ -148,9 +148,9 @@ virInterfaceObjListClone(virInterfaceObjListPtr src, } VIR_FREE(xml); - if ((iface = virInterfaceObjAssignDef(dest, backup)) == NULL) + if ((obj = virInterfaceObjAssignDef(dest, backup)) == NULL) goto cleanup; - virInterfaceObjUnlock(iface); /* locked by virInterfaceObjAssignDef */ + virInterfaceObjUnlock(obj); /* locked by virInterfaceObjAssignDef */ } ret = cnt; @@ -165,47 +165,47 @@ virInterfaceObjPtr virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; - if ((iface = virInterfaceObjFindByName(interfaces, def->name))) { - virInterfaceDefFree(iface->def); - iface->def = def; + if ((obj = virInterfaceObjFindByName(interfaces, def->name))) { + virInterfaceDefFree(obj->def); + obj->def = def; - return iface; + return obj; } - if (VIR_ALLOC(iface) < 0) + if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInit(&iface->lock) < 0) { + if (virMutexInit(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(iface); + VIR_FREE(obj); return NULL; } - virInterfaceObjLock(iface); + virInterfaceObjLock(obj); if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, iface) < 0) { - virInterfaceObjFree(iface); + interfaces->count, obj) < 0) { + virInterfaceObjFree(obj); return NULL; } - iface->def = def; - return iface; + obj->def = def; + return obj; } void virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr iface) + virInterfaceObjPtr obj) { size_t i; - virInterfaceObjUnlock(iface); + virInterfaceObjUnlock(obj); for (i = 0; i < interfaces->count; i++) { virInterfaceObjLock(interfaces->objs[i]); - if (interfaces->objs[i] == iface) { + if (interfaces->objs[i] == obj) { virInterfaceObjUnlock(interfaces->objs[i]); virInterfaceObjFree(interfaces->objs[i]); diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 5b0527d..ee166c6 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -54,7 +54,7 @@ virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, const char *name); void -virInterfaceObjFree(virInterfaceObjPtr iface); +virInterfaceObjFree(virInterfaceObjPtr obj); void virInterfaceObjListFree(virInterfaceObjListPtr vms); @@ -69,7 +69,7 @@ virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, void virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr iface); + virInterfaceObjPtr obj); void virInterfaceObjLock(virInterfaceObjPtr obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2db3f7d..c2697e8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3627,18 +3627,18 @@ static virInterfaceObjPtr testInterfaceObjFindByName(testDriverPtr privconn, const char *name) { - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; testDriverLock(privconn); - iface = virInterfaceObjFindByName(&privconn->ifaces, name); + obj = virInterfaceObjFindByName(&privconn->ifaces, name); testDriverUnlock(privconn); - if (!iface) + if (!obj) virReportError(VIR_ERR_NO_INTERFACE, _("no interface with matching name '%s'"), name); - return iface; + return obj; } @@ -3705,17 +3705,17 @@ testInterfaceLookupByName(virConnectPtr conn, const char *name) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; virInterfacePtr ret = NULL; - if (!(iface = testInterfaceObjFindByName(privconn, name))) + if (!(obj = testInterfaceObjFindByName(privconn, name))) goto cleanup; - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3725,12 +3725,12 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; int ifacect; virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &iface, 1); + ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3743,11 +3743,11 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3869,19 +3869,19 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; char *ret = NULL; virCheckFlags(0, NULL); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - ret = virInterfaceDefFormat(privinterface->def); + ret = virInterfaceDefFormat(obj->def); cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3893,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceDefPtr def; - virInterfaceObjPtr iface = NULL; + virInterfaceObjPtr obj = NULL; virInterfacePtr ret = NULL; virCheckFlags(0, NULL); @@ -3902,16 +3902,16 @@ testInterfaceDefineXML(virConnectPtr conn, if ((def = virInterfaceDefParseString(xmlStr)) == NULL) goto cleanup; - if ((iface = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: virInterfaceDefFree(def); - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); testDriverUnlock(privconn); return ret; } @@ -3921,13 +3921,13 @@ static int testInterfaceUndefine(virInterfacePtr iface) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - virInterfaceObjRemove(&privconn->ifaces, privinterface); + virInterfaceObjRemove(&privconn->ifaces, obj); ret = 0; cleanup: @@ -3940,25 +3940,25 @@ testInterfaceCreate(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; virCheckFlags(0, -1); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - if (privinterface->active != 0) { + if (obj->active != 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - privinterface->active = 1; + obj->active = 1; ret = 0; cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3968,25 +3968,25 @@ testInterfaceDestroy(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; virCheckFlags(0, -1); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - if (privinterface->active == 0) { + if (obj->active == 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - privinterface->active = 0; + obj->active = 0; ret = 0; cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } -- 2.9.4
Rather than using goto cleanup on object find failure and having cleanup need to check if the obj was present before unlocking, just return immediately. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c2697e8..da45542 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3709,13 +3709,11 @@ testInterfaceLookupByName(virConnectPtr conn, virInterfacePtr ret = NULL; if (!(obj = testInterfaceObjFindByName(privconn, name))) - goto cleanup; + return NULL; ret = virGetInterface(conn, obj->def->name, obj->def->mac); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3760,13 +3758,11 @@ testInterfaceIsActive(virInterfacePtr iface) int ret = -1; if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; ret = virInterfaceObjIsActive(obj); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3875,13 +3871,11 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, virCheckFlags(0, NULL); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return NULL; ret = virInterfaceDefFormat(obj->def); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3899,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); testDriverLock(privconn); - if ((def = virInterfaceDefParseString(xmlStr)) == NULL) + if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) @@ -3922,16 +3916,13 @@ testInterfaceUndefine(virInterfacePtr iface) { testDriverPtr privconn = iface->conn->privateData; virInterfaceObjPtr obj; - int ret = -1; if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; virInterfaceObjRemove(&privconn->ifaces, obj); - ret = 0; - cleanup: - return ret; + return 0; } @@ -3946,7 +3937,7 @@ testInterfaceCreate(virInterfacePtr iface, virCheckFlags(0, -1); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; if (obj->active != 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); @@ -3957,8 +3948,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0; cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3974,7 +3964,7 @@ testInterfaceDestroy(virInterfacePtr iface, virCheckFlags(0, -1); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; if (obj->active == 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); @@ -3985,8 +3975,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0; cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } -- 2.9.4
On Fri, May 26, 2017 at 07:59:01 -0400, John Ferlan wrote:
Rather than using goto cleanup on object find failure and having cleanup need to check if the obj was present before unlocking, just return immediately.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c2697e8..da45542 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c
[...]
@@ -3899,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, virCheckFlags(0, NULL);
testDriverLock(privconn); - if ((def = virInterfaceDefParseString(xmlStr)) == NULL) + if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup;
This hunk does something that is not advertised in the commit message. ACK to the rest of the patch.
On 05/26/2017 08:27 AM, Peter Krempa wrote:
On Fri, May 26, 2017 at 07:59:01 -0400, John Ferlan wrote:
Rather than using goto cleanup on object find failure and having cleanup need to check if the obj was present before unlocking, just return immediately.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c2697e8..da45542 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c
[...]
@@ -3899,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, virCheckFlags(0, NULL);
testDriverLock(privconn); - if ((def = virInterfaceDefParseString(xmlStr)) == NULL) + if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup;
This hunk does something that is not advertised in the commit message.
OK - that's just my type-A brain and fingers... I'll convert it back - NBD. Tks - John
ACK to the rest of the patch.
We're about to make the obj much more private, so make it easier to see future changes which will require accessors for the obj->def This also includes modifying some interfaces->objs[i]->X references to be obj = interfaces->objs[i]; and then def = obj->def Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 32 +++++++++++++++++++++----------- src/test/test_driver.c | 12 +++++++++--- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 62c3735..ead9512 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -72,18 +72,21 @@ virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, unsigned int matchct = 0; for (i = 0; i < interfaces->count; i++) { + virInterfaceObjPtr obj = interfaces->objs[i]; + virInterfaceDefPtr def; - virInterfaceObjLock(interfaces->objs[i]); - if (STRCASEEQ(interfaces->objs[i]->def->mac, mac)) { + virInterfaceObjLock(obj); + def = obj->def; + if (STRCASEEQ(def->mac, mac)) { matchct++; if (matchct <= maxmatches) { - matches[matchct - 1] = interfaces->objs[i]; + matches[matchct - 1] = obj; /* keep the lock if we're returning object to caller */ /* it is the caller's responsibility to unlock *all* matches */ continue; } } - virInterfaceObjUnlock(interfaces->objs[i]); + virInterfaceObjUnlock(obj); } return matchct; @@ -97,10 +100,14 @@ virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, size_t i; for (i = 0; i < interfaces->count; i++) { - virInterfaceObjLock(interfaces->objs[i]); - if (STREQ(interfaces->objs[i]->def->name, name)) - return interfaces->objs[i]; - virInterfaceObjUnlock(interfaces->objs[i]); + virInterfaceObjPtr obj = interfaces->objs[i]; + virInterfaceDefPtr def; + + virInterfaceObjLock(obj); + def = obj->def; + if (STREQ(def->name, name)) + return obj; + virInterfaceObjUnlock(obj); } return NULL; @@ -134,10 +141,10 @@ virInterfaceObjListClone(virInterfaceObjListPtr src, virInterfaceObjListFree(dest); /* start with an empty list */ cnt = src->count; for (i = 0; i < cnt; i++) { - virInterfaceDefPtr def = src->objs[i]->def; + virInterfaceObjPtr srcobj = src->objs[i]; virInterfaceDefPtr backup; virInterfaceObjPtr obj; - char *xml = virInterfaceDefFormat(def); + char *xml = virInterfaceDefFormat(srcobj->def); if (!xml) goto cleanup; @@ -247,9 +254,12 @@ virInterfaceObjGetNames(virInterfaceObjListPtr interfaces, for (i = 0; i < interfaces->count && nnames < maxnames; i++) { virInterfaceObjPtr obj = interfaces->objs[i]; + virInterfaceDefPtr def; + virInterfaceObjLock(obj); + def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { - if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { + if (VIR_STRDUP(names[nnames], def->name) < 0) { virInterfaceObjUnlock(obj); goto failure; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index da45542..6ff4657 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3706,12 +3706,14 @@ testInterfaceLookupByName(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; virInterfacePtr ret = NULL; if (!(obj = testInterfaceObjFindByName(privconn, name))) return NULL; + def = obj->def; - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + ret = virGetInterface(conn, def->name, def->mac); virInterfaceObjUnlock(obj); return ret; @@ -3724,6 +3726,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; int ifacect; virInterfacePtr ret = NULL; @@ -3741,7 +3744,8 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + def = obj->def; + ret = virGetInterface(conn, def->name, def->mac); cleanup: if (obj) @@ -3888,6 +3892,7 @@ testInterfaceDefineXML(virConnectPtr conn, testDriverPtr privconn = conn->privateData; virInterfaceDefPtr def; virInterfaceObjPtr obj = NULL; + virInterfaceDefPtr objdef; virInterfacePtr ret = NULL; virCheckFlags(0, NULL); @@ -3899,8 +3904,9 @@ testInterfaceDefineXML(virConnectPtr conn, if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; + objdef = obj->def; - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + ret = virGetInterface(conn, objdef->name, objdef->mac); cleanup: virInterfaceDefFree(def); -- 2.9.4
Move the struct into virinterfaceobj.c, create necessary accessors, and initializers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 28 ++++++++++++++++++++++++++++ src/conf/virinterfaceobj.h | 20 +++++++++----------- src/libvirt_private.syms | 3 +++ src/test/test_driver.c | 20 +++++++++++--------- 4 files changed, 51 insertions(+), 20 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index ead9512..a2ef7f4 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -32,6 +32,12 @@ VIR_LOG_INIT("conf.virinterfaceobj"); +struct _virInterfaceObj { + virMutex lock; + + bool active; /* true if interface is active (up) */ + virInterfaceDefPtr def; /* The interface definition */ +}; /* virInterfaceObj manipulation */ @@ -62,6 +68,28 @@ virInterfaceObjFree(virInterfaceObjPtr obj) } +virInterfaceDefPtr +virInterfaceObjGetDef(virInterfaceObjPtr obj) +{ + return obj->def; +} + + +bool +virInterfaceObjIsActive(virInterfaceObjPtr obj) +{ + return obj->active; +} + + +void +virInterfaceObjSetActive(virInterfaceObjPtr obj, + bool active) +{ + obj->active = active; +} + + /* virInterfaceObjList manipulation */ int virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index ee166c6..79b6fc9 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -24,12 +24,6 @@ typedef struct _virInterfaceObj virInterfaceObj; typedef virInterfaceObj *virInterfaceObjPtr; -struct _virInterfaceObj { - virMutex lock; - - bool active; /* true if interface is active (up) */ - virInterfaceDefPtr def; /* The interface definition */ -}; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr; @@ -38,11 +32,15 @@ struct _virInterfaceObjList { virInterfaceObjPtr *objs; }; -static inline bool -virInterfaceObjIsActive(const virInterfaceObj *iface) -{ - return iface->active; -} +virInterfaceDefPtr +virInterfaceObjGetDef(virInterfaceObjPtr obj); + +bool +virInterfaceObjIsActive(virInterfaceObjPtr obj); + +void +virInterfaceObjSetActive(virInterfaceObjPtr obj, + bool active); int virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d361454..f9fe871 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -913,12 +913,15 @@ virDomainObjListRename; virInterfaceObjAssignDef; virInterfaceObjFindByMACString; virInterfaceObjFindByName; +virInterfaceObjGetDef; virInterfaceObjGetNames; +virInterfaceObjIsActive; virInterfaceObjListClone; virInterfaceObjListFree; virInterfaceObjLock; virInterfaceObjNumOfInterfaces; virInterfaceObjRemove; +virInterfaceObjSetActive; virInterfaceObjUnlock; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6ff4657..412d9f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1025,7 +1025,7 @@ testParseInterfaces(testDriverPtr privconn, goto error; } - obj->active = 1; + virInterfaceObjSetActive(obj, true); virInterfaceObjUnlock(obj); } @@ -3711,7 +3711,7 @@ testInterfaceLookupByName(virConnectPtr conn, if (!(obj = testInterfaceObjFindByName(privconn, name))) return NULL; - def = obj->def; + def = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, def->name, def->mac); @@ -3744,7 +3744,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - def = obj->def; + def = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, def->name, def->mac); cleanup: @@ -3870,14 +3870,16 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, { testDriverPtr privconn = iface->conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; char *ret = NULL; virCheckFlags(0, NULL); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return NULL; + def = virInterfaceObjGetDef(obj); - ret = virInterfaceDefFormat(obj->def); + ret = virInterfaceDefFormat(def); virInterfaceObjUnlock(obj); return ret; @@ -3904,7 +3906,7 @@ testInterfaceDefineXML(virConnectPtr conn, if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; - objdef = obj->def; + objdef = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, objdef->name, objdef->mac); @@ -3945,12 +3947,12 @@ testInterfaceCreate(virInterfacePtr iface, if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - if (obj->active != 0) { + if (virInterfaceObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - obj->active = 1; + virInterfaceObjSetActive(obj, true); ret = 0; cleanup: @@ -3972,12 +3974,12 @@ testInterfaceDestroy(virInterfacePtr iface, if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - if (obj->active == 0) { + if (!virInterfaceObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - obj->active = 0; + virInterfaceObjSetActive(obj, false); ret = 0; cleanup: -- 2.9.4
Move the structs into virinterfaceobj.c, create necessary accessors, and initializers. This also includes reworking virInterfaceObjListClone to handle receiving a source interfaces list pointer, creating the destination interfaces object, and copying everything from source into dest. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 57 +++++++++++++++++++++++++++++----------------- src/conf/virinterfaceobj.h | 12 ++++------ src/libvirt_private.syms | 1 + src/test/test_driver.c | 38 +++++++++++++++---------------- 4 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index a2ef7f4..dd86151 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -39,6 +39,10 @@ struct _virInterfaceObj { virInterfaceDefPtr def; /* The interface definition */ }; +struct _virInterfaceObjList { + size_t count; + virInterfaceObjPtr *objs; +}; /* virInterfaceObj manipulation */ @@ -91,6 +95,17 @@ virInterfaceObjSetActive(virInterfaceObjPtr obj, /* virInterfaceObjList manipulation */ +virInterfaceObjListPtr +virInterfaceObjListNew(void) +{ + virInterfaceObjListPtr interfaces; + + if (VIR_ALLOC(interfaces) < 0) + return NULL; + return interfaces; +} + + int virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, @@ -149,50 +164,50 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces) for (i = 0; i < interfaces->count; i++) virInterfaceObjFree(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - interfaces->count = 0; + VIR_FREE(interfaces); } -int -virInterfaceObjListClone(virInterfaceObjListPtr src, - virInterfaceObjListPtr dest) +virInterfaceObjListPtr +virInterfaceObjListClone(virInterfaceObjListPtr interfaces) { - int ret = -1; size_t i; unsigned int cnt; + virInterfaceObjListPtr dest; - if (!src || !dest) - goto cleanup; + if (!interfaces) + return NULL; - virInterfaceObjListFree(dest); /* start with an empty list */ - cnt = src->count; + if (!(dest = virInterfaceObjListNew())) + return NULL; + + cnt = interfaces->count; for (i = 0; i < cnt; i++) { - virInterfaceObjPtr srcobj = src->objs[i]; + virInterfaceObjPtr srcobj = interfaces->objs[i]; virInterfaceDefPtr backup; virInterfaceObjPtr obj; char *xml = virInterfaceDefFormat(srcobj->def); if (!xml) - goto cleanup; + goto error; - if ((backup = virInterfaceDefParseString(xml)) == NULL) { + if (!(backup = virInterfaceDefParseString(xml))) { VIR_FREE(xml); - goto cleanup; + goto error; } VIR_FREE(xml); - if ((obj = virInterfaceObjAssignDef(dest, backup)) == NULL) - goto cleanup; + if (!(obj = virInterfaceObjAssignDef(dest, backup))) + goto error; virInterfaceObjUnlock(obj); /* locked by virInterfaceObjAssignDef */ } - ret = cnt; - cleanup: - if ((ret < 0) && dest) - virInterfaceObjListFree(dest); - return ret; + return dest; + + error: + virInterfaceObjListFree(dest); + return NULL; } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 79b6fc9..19c4947 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -27,10 +27,6 @@ typedef virInterfaceObj *virInterfaceObjPtr; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr; -struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; -}; virInterfaceDefPtr virInterfaceObjGetDef(virInterfaceObjPtr obj); @@ -42,6 +38,9 @@ void virInterfaceObjSetActive(virInterfaceObjPtr obj, bool active); +virInterfaceObjListPtr +virInterfaceObjListNew(void); + int virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, @@ -57,9 +56,8 @@ virInterfaceObjFree(virInterfaceObjPtr obj); void virInterfaceObjListFree(virInterfaceObjListPtr vms); -int -virInterfaceObjListClone(virInterfaceObjListPtr src, - virInterfaceObjListPtr dest); +virInterfaceObjListPtr +virInterfaceObjListClone(virInterfaceObjListPtr interfaces); virInterfaceObjPtr virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f9fe871..4ba99ad 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -918,6 +918,7 @@ virInterfaceObjGetNames; virInterfaceObjIsActive; virInterfaceObjListClone; virInterfaceObjListFree; +virInterfaceObjListNew; virInterfaceObjLock; virInterfaceObjNumOfInterfaces; virInterfaceObjRemove; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 412d9f1..2cd55ec 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -97,9 +97,9 @@ struct _testDriver { virMutex lock; virNodeInfo nodeInfo; - virInterfaceObjList ifaces; + virInterfaceObjListPtr ifaces; bool transaction_running; - virInterfaceObjList backupIfaces; + virInterfaceObjListPtr backupIfaces; virStoragePoolObjList pools; virNodeDeviceObjList devs; int numCells; @@ -154,7 +154,7 @@ testDriverFree(testDriverPtr driver) virObjectUnref(driver->domains); virNodeDeviceObjListFree(&driver->devs); virObjectUnref(driver->networks); - virInterfaceObjListFree(&driver->ifaces); + virInterfaceObjListFree(driver->ifaces); virStoragePoolObjListFree(&driver->pools); virObjectUnref(driver->eventState); virMutexUnlock(&driver->lock); @@ -416,6 +416,7 @@ testDriverNew(void) if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, &ns)) || !(ret->eventState = virObjectEventStateNew()) || + !(ret->ifaces = virInterfaceObjListNew()) || !(ret->domains = virDomainObjListNew()) || !(ret->networks = virNetworkObjListNew())) goto error; @@ -1020,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjAssignDef(&privconn->ifaces, def))) { + if (!(obj = virInterfaceObjAssignDef(privconn->ifaces, def))) { virInterfaceDefFree(def); goto error; } @@ -3630,7 +3631,7 @@ testInterfaceObjFindByName(testDriverPtr privconn, virInterfaceObjPtr obj; testDriverLock(privconn); - obj = virInterfaceObjFindByName(&privconn->ifaces, name); + obj = virInterfaceObjFindByName(privconn->ifaces, name); testDriverUnlock(privconn); if (!obj) @@ -3649,7 +3650,7 @@ testConnectNumOfInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(&privconn->ifaces, true); + ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, true); testDriverUnlock(privconn); return ninterfaces; } @@ -3664,7 +3665,7 @@ testConnectListInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(&privconn->ifaces, true, names, maxnames); + nnames = virInterfaceObjGetNames(privconn->ifaces, true, names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3678,7 +3679,7 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(&privconn->ifaces, false); + ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, false); testDriverUnlock(privconn); return ninterfaces; } @@ -3693,7 +3694,7 @@ testConnectListDefinedInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(&privconn->ifaces, false, names, maxnames); + nnames = virInterfaceObjGetNames(privconn->ifaces, false, names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3731,7 +3732,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjFindByMACString(privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3789,8 +3790,7 @@ testInterfaceChangeBegin(virConnectPtr conn, privconn->transaction_running = true; - if (virInterfaceObjListClone(&privconn->ifaces, - &privconn->backupIfaces) < 0) + if (!(privconn->backupIfaces = virInterfaceObjListClone(privconn->ifaces))) goto cleanup; ret = 0; @@ -3818,7 +3818,7 @@ testInterfaceChangeCommit(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(&privconn->backupIfaces); + virInterfaceObjListFree(privconn->backupIfaces); privconn->transaction_running = false; ret = 0; @@ -3848,11 +3848,9 @@ testInterfaceChangeRollback(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(&privconn->ifaces); - privconn->ifaces.count = privconn->backupIfaces.count; - privconn->ifaces.objs = privconn->backupIfaces.objs; - privconn->backupIfaces.count = 0; - privconn->backupIfaces.objs = NULL; + virInterfaceObjListFree(privconn->ifaces); + privconn->ifaces = privconn->backupIfaces; + privconn->backupIfaces = NULL; privconn->transaction_running = false; @@ -3903,7 +3901,7 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjAssignDef(privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; objdef = virInterfaceObjGetDef(obj); @@ -3928,7 +3926,7 @@ testInterfaceUndefine(virInterfacePtr iface) if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - virInterfaceObjRemove(&privconn->ifaces, obj); + virInterfaceObjRemove(privconn->ifaces, obj); return 0; } -- 2.9.4
Prefix should have been virInterfaceObjList since the API is operating on the list of interfaces. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 37 +++++++++++++++++++------------------ src/conf/virinterfaceobj.h | 31 ++++++++++++++++--------------- src/libvirt_private.syms | 12 ++++++------ src/test/test_driver.c | 20 +++++++++++--------- 4 files changed, 52 insertions(+), 48 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index dd86151..9470b4a 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -107,9 +107,10 @@ virInterfaceObjListNew(void) int -virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, - const char *mac, - virInterfaceObjPtr *matches, int maxmatches) +virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, + const char *mac, + virInterfaceObjPtr *matches, + int maxmatches) { size_t i; unsigned int matchct = 0; @@ -137,8 +138,8 @@ virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr -virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, - const char *name) +virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, + const char *name) { size_t i; @@ -198,9 +199,9 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) } VIR_FREE(xml); - if (!(obj = virInterfaceObjAssignDef(dest, backup))) + if (!(obj = virInterfaceObjListAssignDef(dest, backup))) goto error; - virInterfaceObjUnlock(obj); /* locked by virInterfaceObjAssignDef */ + virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef */ } return dest; @@ -212,12 +213,12 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) virInterfaceObjPtr -virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def) +virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, + virInterfaceDefPtr def) { virInterfaceObjPtr obj; - if ((obj = virInterfaceObjFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; @@ -247,8 +248,8 @@ virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, void -virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr obj) +virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, + virInterfaceObjPtr obj) { size_t i; @@ -268,8 +269,8 @@ virInterfaceObjRemove(virInterfaceObjListPtr interfaces, int -virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces, - bool wantActive) +virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, + bool wantActive) { size_t i; int ninterfaces = 0; @@ -287,10 +288,10 @@ virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces, int -virInterfaceObjGetNames(virInterfaceObjListPtr interfaces, - bool wantActive, - char **const names, - int maxnames) +virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, + bool wantActive, + char **const names, + int maxnames) { int nnames = 0; size_t i; diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 19c4947..f1bcab9 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -42,13 +42,14 @@ virInterfaceObjListPtr virInterfaceObjListNew(void); int -virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, - const char *mac, - virInterfaceObjPtr *matches, int maxmatches); +virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, + const char *mac, + virInterfaceObjPtr *matches, + int maxmatches); virInterfaceObjPtr -virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, - const char *name); +virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, + const char *name); void virInterfaceObjFree(virInterfaceObjPtr obj); @@ -60,12 +61,12 @@ virInterfaceObjListPtr virInterfaceObjListClone(virInterfaceObjListPtr interfaces); virInterfaceObjPtr -virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def); +virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, + virInterfaceDefPtr def); void -virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr obj); +virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, + virInterfaceObjPtr obj); void virInterfaceObjLock(virInterfaceObjPtr obj); @@ -78,13 +79,13 @@ typedef bool virInterfaceDefPtr def); int -virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces, - bool wantActive); +virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, + bool wantActive); int -virInterfaceObjGetNames(virInterfaceObjListPtr interfaces, - bool wantActive, - char **const names, - int maxnames); +virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, + bool wantActive, + char **const names, + int maxnames); #endif /* __VIRINTERFACEOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ba99ad..aa6f351 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -910,18 +910,18 @@ virDomainObjListRename; # conf/virinterfaceobj.h -virInterfaceObjAssignDef; -virInterfaceObjFindByMACString; -virInterfaceObjFindByName; virInterfaceObjGetDef; -virInterfaceObjGetNames; virInterfaceObjIsActive; +virInterfaceObjListAssignDef; virInterfaceObjListClone; +virInterfaceObjListFindByMACString; +virInterfaceObjListFindByName; virInterfaceObjListFree; +virInterfaceObjListGetNames; virInterfaceObjListNew; +virInterfaceObjListNumOfInterfaces; +virInterfaceObjListRemove; virInterfaceObjLock; -virInterfaceObjNumOfInterfaces; -virInterfaceObjRemove; virInterfaceObjSetActive; virInterfaceObjUnlock; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2cd55ec..89a705c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1021,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjAssignDef(privconn->ifaces, def))) { + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) { virInterfaceDefFree(def); goto error; } @@ -3631,7 +3631,7 @@ testInterfaceObjFindByName(testDriverPtr privconn, virInterfaceObjPtr obj; testDriverLock(privconn); - obj = virInterfaceObjFindByName(privconn->ifaces, name); + obj = virInterfaceObjListFindByName(privconn->ifaces, name); testDriverUnlock(privconn); if (!obj) @@ -3650,7 +3650,7 @@ testConnectNumOfInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, true); + ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, true); testDriverUnlock(privconn); return ninterfaces; } @@ -3665,7 +3665,8 @@ testConnectListInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(privconn->ifaces, true, names, maxnames); + nnames = virInterfaceObjListGetNames(privconn->ifaces, true, + names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3679,7 +3680,7 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, false); + ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, false); testDriverUnlock(privconn); return ninterfaces; } @@ -3694,7 +3695,8 @@ testConnectListDefinedInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(privconn->ifaces, false, names, maxnames); + nnames = virInterfaceObjListGetNames(privconn->ifaces, false, + names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3732,7 +3734,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3901,7 +3903,7 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjAssignDef(privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; objdef = virInterfaceObjGetDef(obj); @@ -3926,7 +3928,7 @@ testInterfaceUndefine(virInterfacePtr iface) if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - virInterfaceObjRemove(privconn->ifaces, obj); + virInterfaceObjListRemove(privconn->ifaces, obj); return 0; } -- 2.9.4
Alter the algorithm to return a list of matching names rather than a list of match virInterfaceObjPtr which are then just dereferenced extracting the def->name and def->mac. Since the def->mac would be the same as the passed @mac, just return a list of names and as long as there's only one, extract the [0] entry from the passed list. Also alter the error message on failure to include the mac that wasn't found. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 23 ++++++++++++++--------- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 16 ++++++++-------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 9470b4a..8bd8094 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -109,11 +109,11 @@ virInterfaceObjListNew(void) int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches) { size_t i; - unsigned int matchct = 0; + int matchct = 0; for (i = 0; i < interfaces->count; i++) { virInterfaceObjPtr obj = interfaces->objs[i]; @@ -122,18 +122,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { - matchct++; - if (matchct <= maxmatches) { - matches[matchct - 1] = obj; - /* keep the lock if we're returning object to caller */ - /* it is the caller's responsibility to unlock *all* matches */ - continue; + if (matchct < maxmatches) { + if (VIR_STRDUP(matches[matchct], def->name) < 0) { + virInterfaceObjUnlock(obj); + goto error; + } + matchct++; } } virInterfaceObjUnlock(obj); - } return matchct; + + error: + while (--matchct >= 0) + VIR_FREE(matches[matchct]); + + return -1; } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index f1bcab9..3934e63 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -44,7 +44,7 @@ virInterfaceObjListNew(void); int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches); virInterfaceObjPtr diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89a705c..ac16f4f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr obj; - virInterfaceDefPtr def; int ifacect; + char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2); testDriverUnlock(privconn); if (ifacect == 0) { - virReportError(VIR_ERR_NO_INTERFACE, NULL); + virReportError(VIR_ERR_NO_INTERFACE, + _("no interface with matching mac '%s'"), mac); goto cleanup; } @@ -3747,12 +3748,11 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - def = virInterfaceObjGetDef(obj); - ret = virGetInterface(conn, def->name, def->mac); + ret = virGetInterface(conn, ifacenames[0], mac); cleanup: - if (obj) - virInterfaceObjUnlock(obj); + VIR_FREE(ifacenames[0]); + VIR_FREE(ifacenames[1]); return ret; } -- 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
For now it'll just call the virInterfaceObjUnlock, but a future adjustment will do something different. The virInterfaceObjUnlock is now private to virinterfaceobj.c with a short term forward reference. Additionally, make virInterfaceObjLock private since it's only used in virinterfaceobj anyway. For now this will involved creating a forward reference, but this will go away soon too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 17 +++++++++++++++-- src/conf/virinterfaceobj.h | 9 +++------ src/libvirt_private.syms | 3 +-- src/test/test_driver.c | 15 +++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1e3f25c..8f839b3 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -44,6 +44,9 @@ struct _virInterfaceObjList { virInterfaceObjPtr *objs; }; +static void +virInterfaceObjLock(virInterfaceObjPtr obj); + /* virInterfaceObj manipulation */ static virInterfaceObjPtr @@ -67,14 +70,14 @@ virInterfaceObjNew(void) } -void +static void virInterfaceObjLock(virInterfaceObjPtr obj) { virMutexLock(&obj->lock); } -void +static void virInterfaceObjUnlock(virInterfaceObjPtr obj) { virMutexUnlock(&obj->lock); @@ -82,6 +85,16 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj) void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) +{ + if (!*obj) + return; + + virInterfaceObjUnlock(*obj); +} + + +void virInterfaceObjFree(virInterfaceObjPtr obj) { if (!obj) 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 aa6f351..364b32e 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 ac16f4f..fb95319 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
On Fri, May 26, 2017 at 07:59:08 -0400, John Ferlan wrote:
For now it'll just call the virInterfaceObjUnlock, but a future adjustment will do something different.
The virInterfaceObjUnlock is now private to virinterfaceobj.c with a short term forward reference.
Additionally, make virInterfaceObjLock private since it's only used in virinterfaceobj anyway. For now this will involved creating a forward reference, but this will go away soon too.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 17 +++++++++++++++-- src/conf/virinterfaceobj.h | 9 +++------ src/libvirt_private.syms | 3 +-- src/test/test_driver.c | 15 +++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-)
[...]
@@ -82,6 +85,16 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj)
void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) +{ + if (!*obj) + return; + + virInterfaceObjUnlock(*obj);
The double pointer is really pointless in this function. Did you forget to set it to NULL after the unlock as we do in the other EndAPI functions?
On 05/26/2017 08:56 AM, Peter Krempa wrote:
On Fri, May 26, 2017 at 07:59:08 -0400, John Ferlan wrote:
For now it'll just call the virInterfaceObjUnlock, but a future adjustment will do something different.
The virInterfaceObjUnlock is now private to virinterfaceobj.c with a short term forward reference.
Additionally, make virInterfaceObjLock private since it's only used in virinterfaceobj anyway. For now this will involved creating a forward reference, but this will go away soon too.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 17 +++++++++++++++-- src/conf/virinterfaceobj.h | 9 +++------ src/libvirt_private.syms | 3 +-- src/test/test_driver.c | 15 +++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-)
[...]
@@ -82,6 +85,16 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj)
void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) +{ + if (!*obj) + return; + + virInterfaceObjUnlock(*obj);
The double pointer is really pointless in this function. Did you forget to set it to NULL after the unlock as we do in the other EndAPI functions?
Well... It was a way to avoid having more changes in the "next" patch. The reality is this and the next one were one at one time, but I figured I could extract out one and make the following one a bit easier to read. I can combine them into one if that's desired. Tks - John
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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 110 ++++++++++++++++++++++----------------------- 2 files changed, 53 insertions(+), 58 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 5077857..4aac3bc 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 8f839b3..51c3c82 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 */ @@ -44,65 +44,62 @@ struct _virInterfaceObjList { virInterfaceObjPtr *objs; }; -static void -virInterfaceObjLock(virInterfaceObjPtr obj); - /* virInterfaceObj manipulation */ -static virInterfaceObjPtr -virInterfaceObjNew(void) -{ - virInterfaceObjPtr obj; +static virClassPtr virInterfaceObjClass; +static void virInterfaceObjDispose(void *obj); - if (VIR_ALLOC(obj) < 0) - return NULL; +static int +virInterfaceObjOnceInit(void) +{ + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + "virInterfaceObj", + sizeof(virInterfaceObj), + virInterfaceObjDispose))) + return -1; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); - return NULL; - } + return 0; +} - virInterfaceObjLock(obj); - return obj; -} +VIR_ONCE_GLOBAL_INIT(virInterfaceObj) static void -virInterfaceObjLock(virInterfaceObjPtr obj) +virInterfaceObjDispose(void *opaque) { - virMutexLock(&obj->lock); + virInterfaceObjPtr obj = opaque; + + virInterfaceDefFree(obj->def); } -static void -virInterfaceObjUnlock(virInterfaceObjPtr obj) +static virInterfaceObjPtr +virInterfaceObjNew(void) { - virMutexUnlock(&obj->lock); -} + virInterfaceObjPtr obj; + if (virInterfaceObjInitialize() < 0) + return NULL; -void -virInterfaceObjEndAPI(virInterfaceObjPtr *obj) -{ - if (!*obj) - return; + if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + return NULL; - virInterfaceObjUnlock(*obj); + virObjectLock(obj); + + return obj; } void -virInterfaceObjFree(virInterfaceObjPtr obj) +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) { - if (!obj) + if (!*obj) return; - virInterfaceDefFree(obj->def); - virMutexDestroy(&obj->lock); - VIR_FREE(obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; } @@ -153,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; @@ -186,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; @@ -203,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); } @@ -240,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; @@ -269,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); } @@ -286,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]); VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } } @@ -310,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; @@ -333,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; -- 2.9.4
Rather than pass by value, let's pass by reference since the object ends up "owning" the XML definition, let's make that ownership a bit more real. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 12 +++++++----- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 51c3c82..f7352d2 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -235,8 +235,10 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) } VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) + if (!(obj = virInterfaceObjListAssignDef(dest, &backup))) { + virInterfaceDefFree(backup); goto error; + } virInterfaceObjEndAPI(&obj); } @@ -250,13 +252,13 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) virInterfaceObjPtr virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def) + virInterfaceDefPtr *def) { virInterfaceObjPtr obj; - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByName(interfaces, (*def)->name))) { virInterfaceDefFree(obj->def); - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return obj; } @@ -270,7 +272,7 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return NULL; } - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return virObjectRef(obj); } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 2b9e1b2..0000ee9 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -65,7 +65,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces); virInterfaceObjPtr virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def); + virInterfaceDefPtr *def); void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index fb95319..4b4a782 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1021,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) { + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, &def))) { virInterfaceDefFree(def); goto error; } @@ -3903,9 +3903,8 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, &def))) goto cleanup; - def = NULL; objdef = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, objdef->name, objdef->mac); -- 2.9.4
On Fri, May 26, 2017 at 07:59:10 -0400, John Ferlan wrote:
Rather than pass by value, let's pass by reference since the object ends up "owning" the XML definition, let's make that ownership a bit more real.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 12 +++++++----- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-)
As said in the previous series. This is introducing a lot of complexity tue to the double pointers and really is not very helpful in making the code understandable.
On 05/26/2017 08:55 AM, Peter Krempa wrote:
On Fri, May 26, 2017 at 07:59:10 -0400, John Ferlan wrote:
Rather than pass by value, let's pass by reference since the object ends up "owning" the XML definition, let's make that ownership a bit more real.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 12 +++++++----- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-)
As said in the previous series. This is introducing a lot of complexity tue to the double pointers and really is not very helpful in making the code understandable.
As in nodedev, I can drop this one. Dropping one at the end of these series is a whole lot easier that one earlier when it comes to merge conflict resolution. John
participants (2)
-
John Ferlan -
Peter Krempa