[libvirt] [PATCH v3 0/9] Multiple cleanups within interfaceobj and interface driver

[libvirt] [PATCH v2 00/14] Work...

[libvirt] [PATCH 00/18] Allow...

John Ferlan

Tuesday, 30 May 2017 Tue, 30 May '17

5:43 a.m.

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

Show replies by date

John Ferlan

Tuesday, 30 May Tue, 30 May

5:43 a.m.

New subject: [libvirt] [PATCH v3 1/9] interface: Consistently use 'obj' for a virInterfaceObjPtr

Alter variable names to be obj rather than 'iface' and/or 'obj'. Signed-off-by: John Ferlan <jferlan(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 2/9] interface: Remove some unnecessary goto's for Interface tests

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(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 3/9] interface: Use virInterfaceDefPtr rather than deref from virInterfaceObjPtr

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(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 4/9] interface: Make _virInterfaceObj struct private

Move the struct into virinterfaceobj.c, create necessary accessors, and initializers. Signed-off-by: John Ferlan <jferlan(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 5/9] interface: Make _virInterfaceObjList struct private

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(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 6/9] interface: Rename some virInterfaceObj* API's

Prefix should have been virInterfaceObjList since the API is operating on the list of interfaces. Signed-off-by: John Ferlan <jferlan(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 7/9] interface: Clean up virInterfaceObjListFindByMACString

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(a)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

John Ferlan

5:43 a.m.

New subject: [libvirt] [PATCH v3 8/9] interface: Introduce virInterfaceObjNew

Create/use a helper to perform the object allocation Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 8bd8094..1e3f25c 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -46,6 +46,27 @@ struct _virInterfaceObjList { /* virInterfaceObj manipulation */ +static virInterfaceObjPtr +virInterfaceObjNew(void) +{ + virInterfaceObjPtr obj; + + if (VIR_ALLOC(obj) < 0) + return NULL; + + if (virMutexInit(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize mutex")); + VIR_FREE(obj); + return NULL; + } + + virInterfaceObjLock(obj); + + return obj; +} + + void virInterfaceObjLock(virInterfaceObjPtr obj) { @@ -230,18 +251,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return obj; } - if (VIR_ALLOC(obj) < 0) - return NULL; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); + if (!(obj = virInterfaceObjNew())) return NULL; - } - virInterfaceObjLock(obj); if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { + virInterfaceObjUnlock(obj); virInterfaceObjFree(obj); return NULL; } -- 2.9.4

Bjoern Walk

7 a.m.

New subject: [libvirt] [PATCH v3 8/9] interface: Introduce virInterfaceObjNew

John Ferlan <jferlan(a)redhat.com> [2017-05-30, 06:43AM -0400]:

...

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(a)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

John Ferlan

5:43 a.m.

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

Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock. This commit also introduces virInterfaceObjEndAPI in order to handle the lock unlock and object unref in one call for consumers returning a NULL obj upon return. This removes the need for virInterfaceObj{Lock|Unlock} external API's. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 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

Bjoern Walk

7:13 a.m.

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

John Ferlan <jferlan(a)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(a)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(a)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

John Ferlan

9:21 a.m.

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

On 05/30/2017 08:13 AM, Bjoern Walk wrote:

...

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

John Ferlan

Saturday, 3 June Sat, 3 Jun

5:41 a.m.

On 05/30/2017 06:43 AM, John Ferlan wrote:

...

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

2728

days inactive

2732

days old

devel@lists.libvirt.org

Manage subscription

13 comments

2 participants

tags (0)

participants (2)

Bjoern Walk
John Ferlan

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v3 0/9] Multiple cleanups within interfaceobj and interface driver