[libvirt] [PATCH v3 0/9] Multiple cleanups within interfaceobj and interface driver
v2: https://www.redhat.com/archives/libvir-list/2017-May/msg01059.html v1: https://www.redhat.com/archives/libvir-list/2017-April/msg01225.html Patches 1-7 were acked in v1 with a request to "show" the next step in a 9/8 patch to convert to using virObject for virInterfaceObj. v2 added that, but also added a couple of patches that were unfavorable. Since I didn't want to push something partial and it was later in the release cycle, I didn't push 1-7. This series keeps the v1 patch 8 in tact and adds patch9 which does the virObject conversion and the virInterfaceObjEndAPI in the same patch (v2 separated those, but it seeems Peter wasn't favoring that option, so I combined them). I know this is not 3.4 material, but with a successful review of 8 & 9, I would push this after 3.4 is cut. John Ferlan (9): 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: Convert virInterfaceObj to use virObjectLockable po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 275 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 72 ++++++------ src/libvirt_private.syms | 19 ++-- src/test/test_driver.c | 142 ++++++++++++----------- 5 files changed, 292 insertions(+), 217 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 9330d9e..d9f9329 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 | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d9f9329..8f7ff63 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; } @@ -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
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 8f7ff63..29c31ad 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 429b095..0929728 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 29c31ad..1b6063a 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 0929728..bdf4eeb 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 1b6063a..7cf0d43 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)) == NULL) 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 bdf4eeb..9be50cb 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 7cf0d43..8444e91 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)) == NULL) 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 8444e91..511d65f 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
John Ferlan <jferlan@redhat.com> [2017-05-30, 06:43AM -0400]:
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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Looks good to me. -- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
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 | 105 ++++++++++++++++++++++++--------------------- src/conf/virinterfaceobj.h | 9 ++-- src/libvirt_private.syms | 3 +- src/test/test_driver.c | 15 +++---- 5 files changed, 68 insertions(+), 65 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 1e3f25c..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 */ @@ -46,50 +46,60 @@ struct _virInterfaceObjList { /* 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) -void -virInterfaceObjLock(virInterfaceObjPtr obj) +static void +virInterfaceObjDispose(void *opaque) { - virMutexLock(&obj->lock); + virInterfaceObjPtr obj = opaque; + + virInterfaceDefFree(obj->def); } -void -virInterfaceObjUnlock(virInterfaceObjPtr obj) +static virInterfaceObjPtr +virInterfaceObjNew(void) { - virMutexUnlock(&obj->lock); + virInterfaceObjPtr obj; + + if (virInterfaceObjInitialize() < 0) + return NULL; + + if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + return NULL; + + 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; } @@ -140,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { if (matchct < maxmatches) { if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto error; } matchct++; } } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return matchct; @@ -173,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virInterfaceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces) size_t i; for (i = 0; i < interfaces->count; i++) - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_FREE(interfaces->objs); VIR_FREE(interfaces); } @@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) VIR_FREE(xml); if (!(obj = virInterfaceObjListAssignDef(dest, backup))) goto error; - virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef */ + virInterfaceObjEndAPI(&obj); } return dest; @@ -256,13 +266,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { - virInterfaceObjUnlock(obj); - virInterfaceObjFree(obj); + virInterfaceObjEndAPI(&obj); return NULL; } obj->def = def; - return obj; + return virObjectRef(obj); } @@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, { size_t i; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < interfaces->count; i++) { - virInterfaceObjLock(interfaces->objs[i]); + virObjectLock(interfaces->objs[i]); if (interfaces->objs[i] == obj) { - virInterfaceObjUnlock(interfaces->objs[i]); - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } } @@ -297,10 +306,10 @@ virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, for (i = 0; (i < interfaces->count); i++) { virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceObjLock(obj); + virObjectLock(obj); if (wantActive == virInterfaceObjIsActive(obj)) ninterfaces++; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return ninterfaces; @@ -320,16 +329,16 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { if (VIR_STRDUP(names[nnames], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return nnames; diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 3934e63..2b9e1b2 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -28,6 +28,9 @@ typedef virInterfaceObj *virInterfaceObjPtr; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr; +void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj); + virInterfaceDefPtr virInterfaceObjGetDef(virInterfaceObjPtr obj); @@ -68,12 +71,6 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj); -void -virInterfaceObjLock(virInterfaceObjPtr obj); - -void -virInterfaceObjUnlock(virInterfaceObjPtr obj); - typedef bool (*virInterfaceObjListFilter)(virConnectPtr conn, virInterfaceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9be50cb..5d6cb5e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -910,6 +910,7 @@ virDomainObjListRename; # conf/virinterfaceobj.h +virInterfaceObjEndAPI; virInterfaceObjGetDef; virInterfaceObjIsActive; virInterfaceObjListAssignDef; @@ -921,9 +922,7 @@ virInterfaceObjListGetNames; virInterfaceObjListNew; virInterfaceObjListNumOfInterfaces; virInterfaceObjListRemove; -virInterfaceObjLock; virInterfaceObjSetActive; -virInterfaceObjUnlock; # conf/virnetworkobj.h diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 511d65f..9a1c8a5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1027,7 +1027,7 @@ testParseInterfaces(testDriverPtr privconn, } virInterfaceObjSetActive(obj, true); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); } ret = 0; @@ -3718,7 +3718,7 @@ testInterfaceLookupByName(virConnectPtr conn, ret = virGetInterface(conn, def->name, def->mac); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3769,7 +3769,7 @@ testInterfaceIsActive(virInterfacePtr iface) ret = virInterfaceObjIsActive(obj); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3881,7 +3881,7 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, ret = virInterfaceDefFormat(def); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3912,8 +3912,7 @@ testInterfaceDefineXML(virConnectPtr conn, cleanup: virInterfaceDefFree(def); - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); testDriverUnlock(privconn); return ret; } @@ -3956,7 +3955,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3983,7 +3982,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } -- 2.9.4
John Ferlan <jferlan@redhat.com> [2017-05-30, 06:43AM -0400]:
[...] void -virInterfaceObjFree(virInterfaceObjPtr obj) +virInterfaceObjEndAPI(virInterfaceObjPtr *obj)
Naming is hard, and I don't have any better suggestion. Just wanted to say that the name is, maybe, improvable :)
{ - if (!obj) + if (!*obj) return;
- virInterfaceDefFree(obj->def); - virMutexDestroy(&obj->lock); - VIR_FREE(obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL;
I'm a bit conflicted if this is strictly necessary. In what situation would this bite a consumer if we don't NULL obj? At least in this patch I can't find any.
}
@@ -140,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { if (matchct < maxmatches) { if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto error; } matchct++; } } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return matchct;
@@ -173,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virInterfaceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); }
return NULL; @@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces) size_t i;
for (i = 0; i < interfaces->count; i++) - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_FREE(interfaces->objs); VIR_FREE(interfaces); } @@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) VIR_FREE(xml); if (!(obj = virInterfaceObjListAssignDef(dest, backup))) goto error; - virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef */ + virInterfaceObjEndAPI(&obj); }
return dest; @@ -256,13 +266,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { - virInterfaceObjUnlock(obj); - virInterfaceObjFree(obj); + virInterfaceObjEndAPI(&obj); return NULL; }
obj->def = def; - return obj; + return virObjectRef(obj);
}
@@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, { size_t i;
- virInterfaceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < interfaces->count; i++) { - virInterfaceObjLock(interfaces->objs[i]); + virObjectLock(interfaces->objs[i]); if (interfaces->objs[i] == obj) { - virInterfaceObjUnlock(interfaces->objs[i]); - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]);
Here you could use virInterfaceObjEndAPI if the nulling would be dropped. Small advantage, I know.
VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } }
@@ -297,10 +306,10 @@ virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces,
for (i = 0; (i < interfaces->count); i++) { virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceObjLock(obj); + virObjectLock(obj); if (wantActive == virInterfaceObjIsActive(obj)) ninterfaces++; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); }
return ninterfaces; @@ -320,16 +329,16 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { if (VIR_STRDUP(names[nnames], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); }
return nnames; diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 3934e63..2b9e1b2 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -28,6 +28,9 @@ typedef virInterfaceObj *virInterfaceObjPtr; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr;
+void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj); + virInterfaceDefPtr virInterfaceObjGetDef(virInterfaceObjPtr obj);
@@ -68,12 +71,6 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj);
-void -virInterfaceObjLock(virInterfaceObjPtr obj); - -void -virInterfaceObjUnlock(virInterfaceObjPtr obj); - typedef bool (*virInterfaceObjListFilter)(virConnectPtr conn, virInterfaceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9be50cb..5d6cb5e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -910,6 +910,7 @@ virDomainObjListRename;
# conf/virinterfaceobj.h +virInterfaceObjEndAPI; virInterfaceObjGetDef; virInterfaceObjIsActive; virInterfaceObjListAssignDef; @@ -921,9 +922,7 @@ virInterfaceObjListGetNames; virInterfaceObjListNew; virInterfaceObjListNumOfInterfaces; virInterfaceObjListRemove; -virInterfaceObjLock; virInterfaceObjSetActive; -virInterfaceObjUnlock;
# conf/virnetworkobj.h diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 511d65f..9a1c8a5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1027,7 +1027,7 @@ testParseInterfaces(testDriverPtr privconn, }
virInterfaceObjSetActive(obj, true); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); }
ret = 0; @@ -3718,7 +3718,7 @@ testInterfaceLookupByName(virConnectPtr conn,
ret = virGetInterface(conn, def->name, def->mac);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3769,7 +3769,7 @@ testInterfaceIsActive(virInterfacePtr iface)
ret = virInterfaceObjIsActive(obj);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3881,7 +3881,7 @@ testInterfaceGetXMLDesc(virInterfacePtr iface,
ret = virInterfaceDefFormat(def);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3912,8 +3912,7 @@ testInterfaceDefineXML(virConnectPtr conn,
cleanup: virInterfaceDefFree(def); - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); testDriverUnlock(privconn); return ret; } @@ -3956,7 +3955,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0;
cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3983,7 +3982,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0;
cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Otherwise, looks good to me. -- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 05/30/2017 08:13 AM, Bjoern Walk wrote:
John Ferlan <jferlan@redhat.com> [2017-05-30, 06:43AM -0400]:
[...] void -virInterfaceObjFree(virInterfaceObjPtr obj) +virInterfaceObjEndAPI(virInterfaceObjPtr *obj)
Naming is hard, and I don't have any better suggestion. Just wanted to say that the name is, maybe, improvable :)
It's a common nomenclature for libvirt... virDomainObjEndAPI, virNetworkObjEndAPI, and virSecretObjEndAPI.
{ - if (!obj) + if (!*obj) return;
- virInterfaceDefFree(obj->def); - virMutexDestroy(&obj->lock); - VIR_FREE(obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL;
I'm a bit conflicted if this is strictly necessary. In what situation would this bite a consumer if we don't NULL obj? At least in this patch I can't find any.
Although perhaps true - it's the common way this happens for other vir*ObjEndAPI source code. Since it's theoretically possible an Unref could cause the object to be Disposed (if refcnt == 0), setting *obj = NULL at least "ensures" the caller misusing the object would be a NULL deref rather than referencing something it no longer owned.
}
@@ -140,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { if (matchct < maxmatches) { if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto error; } matchct++; } } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return matchct;
@@ -173,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virInterfaceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); }
return NULL; @@ -190,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces) size_t i;
for (i = 0; i < interfaces->count; i++) - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_FREE(interfaces->objs); VIR_FREE(interfaces); } @@ -227,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) VIR_FREE(xml); if (!(obj = virInterfaceObjListAssignDef(dest, backup))) goto error; - virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef */ + virInterfaceObjEndAPI(&obj); }
return dest; @@ -256,13 +266,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { - virInterfaceObjUnlock(obj); - virInterfaceObjFree(obj); + virInterfaceObjEndAPI(&obj); return NULL; }
obj->def = def; - return obj; + return virObjectRef(obj);
}
@@ -273,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, { size_t i;
- virInterfaceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < interfaces->count; i++) { - virInterfaceObjLock(interfaces->objs[i]); + virObjectLock(interfaces->objs[i]); if (interfaces->objs[i] == obj) { - virInterfaceObjUnlock(interfaces->objs[i]); - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]);
Here you could use virInterfaceObjEndAPI if the nulling would be dropped. Small advantage, I know.
Understood, I suppose this could also have taken the "virInterfaceObjEndAPI(&obj);" option... Still eventually this is changing from a forward linked list removal to a removal from a hash table. For the sake of my local branches, I'd prefer to keep as is, although if there's a strong desire for something different I'm not opposed to adjusting. Thanks for taking a look at the last two! John
VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } }
@@ -297,10 +306,10 @@ virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces,
for (i = 0; (i < interfaces->count); i++) { virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceObjLock(obj); + virObjectLock(obj); if (wantActive == virInterfaceObjIsActive(obj)) ninterfaces++; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); }
return ninterfaces; @@ -320,16 +329,16 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def;
- virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { if (VIR_STRDUP(names[nnames], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); }
return nnames; diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 3934e63..2b9e1b2 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -28,6 +28,9 @@ typedef virInterfaceObj *virInterfaceObjPtr; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr;
+void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj); + virInterfaceDefPtr virInterfaceObjGetDef(virInterfaceObjPtr obj);
@@ -68,12 +71,6 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj);
-void -virInterfaceObjLock(virInterfaceObjPtr obj); - -void -virInterfaceObjUnlock(virInterfaceObjPtr obj); - typedef bool (*virInterfaceObjListFilter)(virConnectPtr conn, virInterfaceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9be50cb..5d6cb5e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -910,6 +910,7 @@ virDomainObjListRename;
# conf/virinterfaceobj.h +virInterfaceObjEndAPI; virInterfaceObjGetDef; virInterfaceObjIsActive; virInterfaceObjListAssignDef; @@ -921,9 +922,7 @@ virInterfaceObjListGetNames; virInterfaceObjListNew; virInterfaceObjListNumOfInterfaces; virInterfaceObjListRemove; -virInterfaceObjLock; virInterfaceObjSetActive; -virInterfaceObjUnlock;
# conf/virnetworkobj.h diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 511d65f..9a1c8a5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1027,7 +1027,7 @@ testParseInterfaces(testDriverPtr privconn, }
virInterfaceObjSetActive(obj, true); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); }
ret = 0; @@ -3718,7 +3718,7 @@ testInterfaceLookupByName(virConnectPtr conn,
ret = virGetInterface(conn, def->name, def->mac);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3769,7 +3769,7 @@ testInterfaceIsActive(virInterfacePtr iface)
ret = virInterfaceObjIsActive(obj);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3881,7 +3881,7 @@ testInterfaceGetXMLDesc(virInterfacePtr iface,
ret = virInterfaceDefFormat(def);
- virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3912,8 +3912,7 @@ testInterfaceDefineXML(virConnectPtr conn,
cleanup: virInterfaceDefFree(def); - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); testDriverUnlock(privconn); return ret; } @@ -3956,7 +3955,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0;
cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
@@ -3983,7 +3982,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0;
cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; }
-- 2.9.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Otherwise, looks good to me.
On 05/30/2017 06:43 AM, John Ferlan wrote:
v2: https://www.redhat.com/archives/libvir-list/2017-May/msg01059.html v1: https://www.redhat.com/archives/libvir-list/2017-April/msg01225.html
Patches 1-7 were acked in v1 with a request to "show" the next step in a 9/8 patch to convert to using virObject for virInterfaceObj. v2 added that, but also added a couple of patches that were unfavorable.
Since I didn't want to push something partial and it was later in the release cycle, I didn't push 1-7. This series keeps the v1 patch 8 in tact and adds patch9 which does the virObject conversion and the virInterfaceObjEndAPI in the same patch (v2 separated those, but it seeems Peter wasn't favoring that option, so I combined them).
I know this is not 3.4 material, but with a successful review of 8 & 9, I would push this after 3.4 is cut.
John Ferlan (9): 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: Convert virInterfaceObj to use virObjectLockable
po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 275 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 72 ++++++------ src/libvirt_private.syms | 19 ++-- src/test/test_driver.c | 142 ++++++++++++----------- 5 files changed, 292 insertions(+), 217 deletions(-)
Based on ACK in v1 (and now that 3.4.0 is out the door), I've pushed the first 7 patches. Although 8-9 were essentially ACK'd - while working through the other vir*obj.c modules (nwfilter, secret, and nodedev) I realized I'd missed a step between 8 and 9 as well a potential leak of the object, so I'll resend 3 as a v4 series shortly. John
participants (2)
-
Bjoern Walk -
John Ferlan