[libvirt] [PATCH 0/8] Multiple cleanups within interfaceobj and interface driver
Sensing a theme lately yet? More adjustments in preparation for having virobject code handle the bulk of the object management code. Far less in this series than the other two... The Clone code was a bit unique and required more of a rewrite than a refigure since now we have two allocated lists rather than two static lists. John Ferlan (8): 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 src/conf/virinterfaceobj.c | 232 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 65 ++++++------- src/libvirt_private.syms | 15 +-- src/test/test_driver.c | 143 ++++++++++++++-------------- 4 files changed, 262 insertions(+), 193 deletions(-) -- 2.9.3
Alter variable names to be obj rather than 'iface' and/or 'obj'. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 48 ++++++++++++++-------------- src/conf/virinterfaceobj.h | 4 +-- src/test/test_driver.c | 78 +++++++++++++++++++++++----------------------- 3 files changed, 65 insertions(+), 65 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e80db23..62c3735 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -51,14 +51,14 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj) void -virInterfaceObjFree(virInterfaceObjPtr iface) +virInterfaceObjFree(virInterfaceObjPtr obj) { - if (!iface) + if (!obj) return; - virInterfaceDefFree(iface->def); - virMutexDestroy(&iface->lock); - VIR_FREE(iface); + virInterfaceDefFree(obj->def); + virMutexDestroy(&obj->lock); + VIR_FREE(obj); } @@ -136,7 +136,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr src, for (i = 0; i < cnt; i++) { virInterfaceDefPtr def = src->objs[i]->def; virInterfaceDefPtr backup; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; char *xml = virInterfaceDefFormat(def); if (!xml) @@ -148,9 +148,9 @@ virInterfaceObjListClone(virInterfaceObjListPtr src, } VIR_FREE(xml); - if ((iface = virInterfaceObjAssignDef(dest, backup)) == NULL) + if ((obj = virInterfaceObjAssignDef(dest, backup)) == NULL) goto cleanup; - virInterfaceObjUnlock(iface); /* locked by virInterfaceObjAssignDef */ + virInterfaceObjUnlock(obj); /* locked by virInterfaceObjAssignDef */ } ret = cnt; @@ -165,47 +165,47 @@ virInterfaceObjPtr virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; - if ((iface = virInterfaceObjFindByName(interfaces, def->name))) { - virInterfaceDefFree(iface->def); - iface->def = def; + if ((obj = virInterfaceObjFindByName(interfaces, def->name))) { + virInterfaceDefFree(obj->def); + obj->def = def; - return iface; + return obj; } - if (VIR_ALLOC(iface) < 0) + if (VIR_ALLOC(obj) < 0) return NULL; - if (virMutexInit(&iface->lock) < 0) { + if (virMutexInit(&obj->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(iface); + VIR_FREE(obj); return NULL; } - virInterfaceObjLock(iface); + virInterfaceObjLock(obj); if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, iface) < 0) { - virInterfaceObjFree(iface); + interfaces->count, obj) < 0) { + virInterfaceObjFree(obj); return NULL; } - iface->def = def; - return iface; + obj->def = def; + return obj; } void virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr iface) + virInterfaceObjPtr obj) { size_t i; - virInterfaceObjUnlock(iface); + virInterfaceObjUnlock(obj); for (i = 0; i < interfaces->count; i++) { virInterfaceObjLock(interfaces->objs[i]); - if (interfaces->objs[i] == iface) { + if (interfaces->objs[i] == obj) { virInterfaceObjUnlock(interfaces->objs[i]); virInterfaceObjFree(interfaces->objs[i]); diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 5b0527d..ee166c6 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -54,7 +54,7 @@ virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, const char *name); void -virInterfaceObjFree(virInterfaceObjPtr iface); +virInterfaceObjFree(virInterfaceObjPtr obj); void virInterfaceObjListFree(virInterfaceObjListPtr vms); @@ -69,7 +69,7 @@ virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, void virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr iface); + virInterfaceObjPtr obj); void virInterfaceObjLock(virInterfaceObjPtr obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2db3f7d..c2697e8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3627,18 +3627,18 @@ static virInterfaceObjPtr testInterfaceObjFindByName(testDriverPtr privconn, const char *name) { - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; testDriverLock(privconn); - iface = virInterfaceObjFindByName(&privconn->ifaces, name); + obj = virInterfaceObjFindByName(&privconn->ifaces, name); testDriverUnlock(privconn); - if (!iface) + if (!obj) virReportError(VIR_ERR_NO_INTERFACE, _("no interface with matching name '%s'"), name); - return iface; + return obj; } @@ -3705,17 +3705,17 @@ testInterfaceLookupByName(virConnectPtr conn, const char *name) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; virInterfacePtr ret = NULL; - if (!(iface = testInterfaceObjFindByName(privconn, name))) + if (!(obj = testInterfaceObjFindByName(privconn, name))) goto cleanup; - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3725,12 +3725,12 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr iface; + virInterfaceObjPtr obj; int ifacect; virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &iface, 1); + ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3743,11 +3743,11 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3869,19 +3869,19 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; char *ret = NULL; virCheckFlags(0, NULL); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - ret = virInterfaceDefFormat(privinterface->def); + ret = virInterfaceDefFormat(obj->def); cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3893,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceDefPtr def; - virInterfaceObjPtr iface = NULL; + virInterfaceObjPtr obj = NULL; virInterfacePtr ret = NULL; virCheckFlags(0, NULL); @@ -3902,16 +3902,16 @@ testInterfaceDefineXML(virConnectPtr conn, if ((def = virInterfaceDefParseString(xmlStr)) == NULL) goto cleanup; - if ((iface = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; - ret = virGetInterface(conn, iface->def->name, iface->def->mac); + ret = virGetInterface(conn, obj->def->name, obj->def->mac); cleanup: virInterfaceDefFree(def); - if (iface) - virInterfaceObjUnlock(iface); + if (obj) + virInterfaceObjUnlock(obj); testDriverUnlock(privconn); return ret; } @@ -3921,13 +3921,13 @@ static int testInterfaceUndefine(virInterfacePtr iface) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - virInterfaceObjRemove(&privconn->ifaces, privinterface); + virInterfaceObjRemove(&privconn->ifaces, obj); ret = 0; cleanup: @@ -3940,25 +3940,25 @@ testInterfaceCreate(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; virCheckFlags(0, -1); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - if (privinterface->active != 0) { + if (obj->active != 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - privinterface->active = 1; + obj->active = 1; ret = 0; cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } @@ -3968,25 +3968,25 @@ testInterfaceDestroy(virInterfacePtr iface, unsigned int flags) { testDriverPtr privconn = iface->conn->privateData; - virInterfaceObjPtr privinterface; + virInterfaceObjPtr obj; int ret = -1; virCheckFlags(0, -1); - if (!(privinterface = testInterfaceObjFindByName(privconn, iface->name))) + if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) goto cleanup; - if (privinterface->active == 0) { + if (obj->active == 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - privinterface->active = 0; + obj->active = 0; ret = 0; cleanup: - if (privinterface) - virInterfaceObjUnlock(privinterface); + if (obj) + virInterfaceObjUnlock(obj); return ret; } -- 2.9.3
Rather than using goto cleanup on object find failure and having cleanup need to check if the obj was present before unlocking, just return immediately. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c2697e8..da45542 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3709,13 +3709,11 @@ testInterfaceLookupByName(virConnectPtr conn, virInterfacePtr ret = NULL; if (!(obj = testInterfaceObjFindByName(privconn, name))) - goto cleanup; + return NULL; ret = virGetInterface(conn, obj->def->name, obj->def->mac); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3760,13 +3758,11 @@ testInterfaceIsActive(virInterfacePtr iface) int ret = -1; if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; ret = virInterfaceObjIsActive(obj); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3875,13 +3871,11 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, virCheckFlags(0, NULL); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return NULL; ret = virInterfaceDefFormat(obj->def); - cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3899,7 +3893,7 @@ testInterfaceDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); testDriverLock(privconn); - if ((def = virInterfaceDefParseString(xmlStr)) == NULL) + if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) @@ -3922,16 +3916,13 @@ testInterfaceUndefine(virInterfacePtr iface) { testDriverPtr privconn = iface->conn->privateData; virInterfaceObjPtr obj; - int ret = -1; if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; virInterfaceObjRemove(&privconn->ifaces, obj); - ret = 0; - cleanup: - return ret; + return 0; } @@ -3946,7 +3937,7 @@ testInterfaceCreate(virInterfacePtr iface, virCheckFlags(0, -1); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; if (obj->active != 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); @@ -3957,8 +3948,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0; cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } @@ -3974,7 +3964,7 @@ testInterfaceDestroy(virInterfacePtr iface, virCheckFlags(0, -1); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) - goto cleanup; + return -1; if (obj->active == 0) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); @@ -3985,8 +3975,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0; cleanup: - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjUnlock(obj); return ret; } -- 2.9.3
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 | 33 +++++++++++++++++++++------------ src/test/test_driver.c | 12 +++++++++--- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 62c3735..36131af 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -55,7 +55,6 @@ virInterfaceObjFree(virInterfaceObjPtr obj) { if (!obj) return; - virInterfaceDefFree(obj->def); virMutexDestroy(&obj->lock); VIR_FREE(obj); @@ -72,18 +71,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 +99,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 +140,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 +253,12 @@ virInterfaceObjGetNames(virInterfaceObjListPtr interfaces, for (i = 0; i < interfaces->count && nnames < maxnames; i++) { virInterfaceObjPtr obj = interfaces->objs[i]; + virInterfaceDefPtr def; + virInterfaceObjLock(obj); + def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { - if (VIR_STRDUP(names[nnames], obj->def->name) < 0) { + if (VIR_STRDUP(names[nnames], def->name) < 0) { virInterfaceObjUnlock(obj); goto failure; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index da45542..6ff4657 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3706,12 +3706,14 @@ testInterfaceLookupByName(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; virInterfacePtr ret = NULL; if (!(obj = testInterfaceObjFindByName(privconn, name))) return NULL; + def = obj->def; - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + ret = virGetInterface(conn, def->name, def->mac); virInterfaceObjUnlock(obj); return ret; @@ -3724,6 +3726,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, { testDriverPtr privconn = conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; int ifacect; virInterfacePtr ret = NULL; @@ -3741,7 +3744,8 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + def = obj->def; + ret = virGetInterface(conn, def->name, def->mac); cleanup: if (obj) @@ -3888,6 +3892,7 @@ testInterfaceDefineXML(virConnectPtr conn, testDriverPtr privconn = conn->privateData; virInterfaceDefPtr def; virInterfaceObjPtr obj = NULL; + virInterfaceDefPtr objdef; virInterfacePtr ret = NULL; virCheckFlags(0, NULL); @@ -3899,8 +3904,9 @@ testInterfaceDefineXML(virConnectPtr conn, if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; + objdef = obj->def; - ret = virGetInterface(conn, obj->def->name, obj->def->mac); + ret = virGetInterface(conn, objdef->name, objdef->mac); cleanup: virInterfaceDefFree(def); -- 2.9.3
On 04/26/2017 12:36 AM, John Ferlan wrote:
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 | 33 +++++++++++++++++++++------------ src/test/test_driver.c | 12 +++++++++--- 2 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 62c3735..36131af 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -55,7 +55,6 @@ virInterfaceObjFree(virInterfaceObjPtr obj) { if (!obj) return; - virInterfaceDefFree(obj->def); virMutexDestroy(&obj->lock); VIR_FREE(obj);
This looks spurious. Michal
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 36131af..68faa66 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 */ @@ -61,6 +67,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 83e979a..751d1ac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -909,12 +909,15 @@ virDomainObjListRename; virInterfaceObjAssignDef; virInterfaceObjFindByMACString; virInterfaceObjFindByName; +virInterfaceObjGetDef; virInterfaceObjGetNames; +virInterfaceObjIsActive; virInterfaceObjListClone; virInterfaceObjListFree; virInterfaceObjLock; virInterfaceObjNumOfInterfaces; virInterfaceObjRemove; +virInterfaceObjSetActive; virInterfaceObjUnlock; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6ff4657..412d9f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1025,7 +1025,7 @@ testParseInterfaces(testDriverPtr privconn, goto error; } - obj->active = 1; + virInterfaceObjSetActive(obj, true); virInterfaceObjUnlock(obj); } @@ -3711,7 +3711,7 @@ testInterfaceLookupByName(virConnectPtr conn, if (!(obj = testInterfaceObjFindByName(privconn, name))) return NULL; - def = obj->def; + def = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, def->name, def->mac); @@ -3744,7 +3744,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - def = obj->def; + def = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, def->name, def->mac); cleanup: @@ -3870,14 +3870,16 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, { testDriverPtr privconn = iface->conn->privateData; virInterfaceObjPtr obj; + virInterfaceDefPtr def; char *ret = NULL; virCheckFlags(0, NULL); if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return NULL; + def = virInterfaceObjGetDef(obj); - ret = virInterfaceDefFormat(obj->def); + ret = virInterfaceDefFormat(def); virInterfaceObjUnlock(obj); return ret; @@ -3904,7 +3906,7 @@ testInterfaceDefineXML(virConnectPtr conn, if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; - objdef = obj->def; + objdef = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, objdef->name, objdef->mac); @@ -3945,12 +3947,12 @@ testInterfaceCreate(virInterfacePtr iface, if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - if (obj->active != 0) { + if (virInterfaceObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - obj->active = 1; + virInterfaceObjSetActive(obj, true); ret = 0; cleanup: @@ -3972,12 +3974,12 @@ testInterfaceDestroy(virInterfacePtr iface, if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - if (obj->active == 0) { + if (!virInterfaceObjIsActive(obj)) { virReportError(VIR_ERR_OPERATION_INVALID, NULL); goto cleanup; } - obj->active = 0; + virInterfaceObjSetActive(obj, false); ret = 0; cleanup: -- 2.9.3
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 | 58 ++++++++++++++++++++++++++++------------------ src/conf/virinterfaceobj.h | 12 ++++------ src/test/test_driver.c | 38 ++++++++++++++---------------- 3 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 68faa66..301ad94 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 */ @@ -90,6 +94,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, @@ -148,50 +163,49 @@ 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/test/test_driver.c b/src/test/test_driver.c index 412d9f1..2cd55ec 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -97,9 +97,9 @@ struct _testDriver { virMutex lock; virNodeInfo nodeInfo; - virInterfaceObjList ifaces; + virInterfaceObjListPtr ifaces; bool transaction_running; - virInterfaceObjList backupIfaces; + virInterfaceObjListPtr backupIfaces; virStoragePoolObjList pools; virNodeDeviceObjList devs; int numCells; @@ -154,7 +154,7 @@ testDriverFree(testDriverPtr driver) virObjectUnref(driver->domains); virNodeDeviceObjListFree(&driver->devs); virObjectUnref(driver->networks); - virInterfaceObjListFree(&driver->ifaces); + virInterfaceObjListFree(driver->ifaces); virStoragePoolObjListFree(&driver->pools); virObjectUnref(driver->eventState); virMutexUnlock(&driver->lock); @@ -416,6 +416,7 @@ testDriverNew(void) if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, &ns)) || !(ret->eventState = virObjectEventStateNew()) || + !(ret->ifaces = virInterfaceObjListNew()) || !(ret->domains = virDomainObjListNew()) || !(ret->networks = virNetworkObjListNew())) goto error; @@ -1020,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjAssignDef(&privconn->ifaces, def))) { + if (!(obj = virInterfaceObjAssignDef(privconn->ifaces, def))) { virInterfaceDefFree(def); goto error; } @@ -3630,7 +3631,7 @@ testInterfaceObjFindByName(testDriverPtr privconn, virInterfaceObjPtr obj; testDriverLock(privconn); - obj = virInterfaceObjFindByName(&privconn->ifaces, name); + obj = virInterfaceObjFindByName(privconn->ifaces, name); testDriverUnlock(privconn); if (!obj) @@ -3649,7 +3650,7 @@ testConnectNumOfInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(&privconn->ifaces, true); + ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, true); testDriverUnlock(privconn); return ninterfaces; } @@ -3664,7 +3665,7 @@ testConnectListInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(&privconn->ifaces, true, names, maxnames); + nnames = virInterfaceObjGetNames(privconn->ifaces, true, names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3678,7 +3679,7 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(&privconn->ifaces, false); + ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, false); testDriverUnlock(privconn); return ninterfaces; } @@ -3693,7 +3694,7 @@ testConnectListDefinedInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(&privconn->ifaces, false, names, maxnames); + nnames = virInterfaceObjGetNames(privconn->ifaces, false, names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3731,7 +3732,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(&privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjFindByMACString(privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3789,8 +3790,7 @@ testInterfaceChangeBegin(virConnectPtr conn, privconn->transaction_running = true; - if (virInterfaceObjListClone(&privconn->ifaces, - &privconn->backupIfaces) < 0) + if (!(privconn->backupIfaces = virInterfaceObjListClone(privconn->ifaces))) goto cleanup; ret = 0; @@ -3818,7 +3818,7 @@ testInterfaceChangeCommit(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(&privconn->backupIfaces); + virInterfaceObjListFree(privconn->backupIfaces); privconn->transaction_running = false; ret = 0; @@ -3848,11 +3848,9 @@ testInterfaceChangeRollback(virConnectPtr conn, goto cleanup; } - virInterfaceObjListFree(&privconn->ifaces); - privconn->ifaces.count = privconn->backupIfaces.count; - privconn->ifaces.objs = privconn->backupIfaces.objs; - privconn->backupIfaces.count = 0; - privconn->backupIfaces.objs = NULL; + virInterfaceObjListFree(privconn->ifaces); + privconn->ifaces = privconn->backupIfaces; + privconn->backupIfaces = NULL; privconn->transaction_running = false; @@ -3903,7 +3901,7 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjAssignDef(&privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjAssignDef(privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; objdef = virInterfaceObjGetDef(obj); @@ -3928,7 +3926,7 @@ testInterfaceUndefine(virInterfacePtr iface) if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - virInterfaceObjRemove(&privconn->ifaces, obj); + virInterfaceObjRemove(privconn->ifaces, obj); return 0; } -- 2.9.3
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 301ad94..3aeeebd 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -106,9 +106,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; @@ -136,8 +137,8 @@ virInterfaceObjFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr -virInterfaceObjFindByName(virInterfaceObjListPtr interfaces, - const char *name) +virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, + const char *name) { size_t i; @@ -196,9 +197,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; @@ -210,12 +211,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; @@ -245,8 +246,8 @@ virInterfaceObjAssignDef(virInterfaceObjListPtr interfaces, void -virInterfaceObjRemove(virInterfaceObjListPtr interfaces, - virInterfaceObjPtr obj) +virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, + virInterfaceObjPtr obj) { size_t i; @@ -266,8 +267,8 @@ virInterfaceObjRemove(virInterfaceObjListPtr interfaces, int -virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces, - bool wantActive) +virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, + bool wantActive) { size_t i; int ninterfaces = 0; @@ -285,10 +286,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 751d1ac..4feb081 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -906,17 +906,17 @@ virDomainObjListRename; # conf/virinterfaceobj.h -virInterfaceObjAssignDef; -virInterfaceObjFindByMACString; -virInterfaceObjFindByName; virInterfaceObjGetDef; -virInterfaceObjGetNames; virInterfaceObjIsActive; +virInterfaceObjListAssignDef; virInterfaceObjListClone; +virInterfaceObjListFindByMACString; +virInterfaceObjListFindByName; virInterfaceObjListFree; +virInterfaceObjListGetNames; +virInterfaceObjListNumOfInterfaces; +virInterfaceObjListRemove; virInterfaceObjLock; -virInterfaceObjNumOfInterfaces; -virInterfaceObjRemove; virInterfaceObjSetActive; virInterfaceObjUnlock; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2cd55ec..89a705c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1021,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjAssignDef(privconn->ifaces, def))) { + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) { virInterfaceDefFree(def); goto error; } @@ -3631,7 +3631,7 @@ testInterfaceObjFindByName(testDriverPtr privconn, virInterfaceObjPtr obj; testDriverLock(privconn); - obj = virInterfaceObjFindByName(privconn->ifaces, name); + obj = virInterfaceObjListFindByName(privconn->ifaces, name); testDriverUnlock(privconn); if (!obj) @@ -3650,7 +3650,7 @@ testConnectNumOfInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, true); + ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, true); testDriverUnlock(privconn); return ninterfaces; } @@ -3665,7 +3665,8 @@ testConnectListInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(privconn->ifaces, true, names, maxnames); + nnames = virInterfaceObjListGetNames(privconn->ifaces, true, + names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3679,7 +3680,7 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn) int ninterfaces; testDriverLock(privconn); - ninterfaces = virInterfaceObjNumOfInterfaces(privconn->ifaces, false); + ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, false); testDriverUnlock(privconn); return ninterfaces; } @@ -3694,7 +3695,8 @@ testConnectListDefinedInterfaces(virConnectPtr conn, int nnames; testDriverLock(privconn); - nnames = virInterfaceObjGetNames(privconn->ifaces, false, names, maxnames); + nnames = virInterfaceObjListGetNames(privconn->ifaces, false, + names, maxnames); testDriverUnlock(privconn); return nnames; @@ -3732,7 +3734,7 @@ testInterfaceLookupByMACString(virConnectPtr conn, virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); testDriverUnlock(privconn); if (ifacect == 0) { @@ -3901,7 +3903,7 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjAssignDef(privconn->ifaces, def)) == NULL) + if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) goto cleanup; def = NULL; objdef = virInterfaceObjGetDef(obj); @@ -3926,7 +3928,7 @@ testInterfaceUndefine(virInterfacePtr iface) if (!(obj = testInterfaceObjFindByName(privconn, iface->name))) return -1; - virInterfaceObjRemove(privconn->ifaces, obj); + virInterfaceObjListRemove(privconn->ifaces, obj); return 0; } -- 2.9.3
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 3aeeebd..1cc5c92 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -108,11 +108,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]; @@ -121,18 +121,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { - matchct++; - if (matchct <= maxmatches) { - matches[matchct - 1] = obj; - /* keep the lock if we're returning object to caller */ - /* it is the caller's responsibility to unlock *all* matches */ - continue; + if (matchct < maxmatches) { + if (VIR_STRDUP(matches[matchct], def->name) < 0) { + virInterfaceObjUnlock(obj); + goto error; + } + matchct++; } } virInterfaceObjUnlock(obj); - } return matchct; + + error: + while (--matchct >= 0) + VIR_FREE(matches[matchct]); + + return -1; } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index f1bcab9..3934e63 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -44,7 +44,7 @@ virInterfaceObjListNew(void); int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches); virInterfaceObjPtr diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89a705c..ac16f4f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr obj; - virInterfaceDefPtr def; int ifacect; + char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL; testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2); testDriverUnlock(privconn); if (ifacect == 0) { - virReportError(VIR_ERR_NO_INTERFACE, NULL); + virReportError(VIR_ERR_NO_INTERFACE, + _("no interface with matching mac '%s'"), mac); goto cleanup; } @@ -3747,12 +3748,11 @@ testInterfaceLookupByMACString(virConnectPtr conn, goto cleanup; } - def = virInterfaceObjGetDef(obj); - ret = virGetInterface(conn, def->name, def->mac); + ret = virGetInterface(conn, ifacenames[0], mac); cleanup: - if (obj) - virInterfaceObjUnlock(obj); + VIR_FREE(ifacenames[0]); + VIR_FREE(ifacenames[1]); return ret; } -- 2.9.3
On 04/26/2017 12:36 AM, John Ferlan wrote:
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 3aeeebd..1cc5c92 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -108,11 +108,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]; @@ -121,18 +121,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { - matchct++; - if (matchct <= maxmatches) { - matches[matchct - 1] = obj; - /* keep the lock if we're returning object to caller */ - /* it is the caller's responsibility to unlock *all* matches */ - continue; + if (matchct < maxmatches) { + if (VIR_STRDUP(matches[matchct], def->name) < 0) { + virInterfaceObjUnlock(obj); + goto error; + } + matchct++; } } virInterfaceObjUnlock(obj); - } return matchct; + + error: + while (--matchct >= 0) + VIR_FREE(matches[matchct]); + + return -1; }
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index f1bcab9..3934e63 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -44,7 +44,7 @@ virInterfaceObjListNew(void); int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches);
virInterfaceObjPtr diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89a705c..ac16f4f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr obj; - virInterfaceDefPtr def; int ifacect; + char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL;
testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2);
ARRAY_CARDINALITY()
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]);
And this can be probably dynamic then: for (i = 0; i < ARRAY_CARDINALITY(ifacenames); i++) VIR_FREE(ifacenames[i]);
return ret; }
Michal
On 05/19/2017 11:29 AM, Michal Privoznik wrote:
On 04/26/2017 12:36 AM, John Ferlan wrote:
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 3aeeebd..1cc5c92 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -108,11 +108,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]; @@ -121,18 +121,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { - matchct++; - if (matchct <= maxmatches) { - matches[matchct - 1] = obj; - /* keep the lock if we're returning object to caller */ - /* it is the caller's responsibility to unlock *all* matches */ - continue; + if (matchct < maxmatches) { + if (VIR_STRDUP(matches[matchct], def->name) < 0) { + virInterfaceObjUnlock(obj); + goto error; + } + matchct++; } } virInterfaceObjUnlock(obj); - } return matchct; + + error: + while (--matchct >= 0) + VIR_FREE(matches[matchct]); + + return -1; }
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index f1bcab9..3934e63 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -44,7 +44,7 @@ virInterfaceObjListNew(void); int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches);
virInterfaceObjPtr diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89a705c..ac16f4f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr obj; - virInterfaceDefPtr def; int ifacect; + char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL;
testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2);
ARRAY_CARDINALITY()
Don't really see the advantage. All this does is try to ensure the provided MAC is "unique" by collecting all interfaces with it defined. Once it reaches 2, FindByMACString won't collect any more since it reached maxmatches. John
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]);
And this can be probably dynamic then:
for (i = 0; i < ARRAY_CARDINALITY(ifacenames); i++) VIR_FREE(ifacenames[i]);
return ret; }
Michal
On 05/20/2017 01:18 PM, John Ferlan wrote:
On 05/19/2017 11:29 AM, Michal Privoznik wrote:
On 04/26/2017 12:36 AM, John Ferlan wrote:
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 3aeeebd..1cc5c92 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -108,11 +108,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]; @@ -121,18 +121,23 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { - matchct++; - if (matchct <= maxmatches) { - matches[matchct - 1] = obj; - /* keep the lock if we're returning object to caller */ - /* it is the caller's responsibility to unlock *all* matches */ - continue; + if (matchct < maxmatches) { + if (VIR_STRDUP(matches[matchct], def->name) < 0) { + virInterfaceObjUnlock(obj); + goto error; + } + matchct++; } } virInterfaceObjUnlock(obj); - } return matchct; + + error: + while (--matchct >= 0) + VIR_FREE(matches[matchct]); + + return -1; }
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index f1bcab9..3934e63 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -44,7 +44,7 @@ virInterfaceObjListNew(void); int virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, const char *mac, - virInterfaceObjPtr *matches, + char **const matches, int maxmatches);
virInterfaceObjPtr diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89a705c..ac16f4f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3728,17 +3728,18 @@ testInterfaceLookupByMACString(virConnectPtr conn, const char *mac) { testDriverPtr privconn = conn->privateData; - virInterfaceObjPtr obj; - virInterfaceDefPtr def; int ifacect; + char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL;
testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, &obj, 1); + ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2);
ARRAY_CARDINALITY()
Don't really see the advantage. All this does is try to ensure the provided MAC is "unique" by collecting all interfaces with it defined. Once it reaches 2, FindByMACString won't collect any more since it reached maxmatches.
The advantage to me is that we get rid of the magical constant. I can immediately see what is "2" supposed to mean. But I agree that the test driver is not our display case. So after all, I don't care that much. I just thought it would be nice to have it. Michal
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 1cc5c92..4463653 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) { @@ -228,18 +249,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.3
On 04/26/2017 12:36 AM, John Ferlan wrote:
Create/use a helper to perform the object allocation
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1cc5c92..4463653 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; +} + +
Any reason why virInterfaceObj can't actually be an virObject? virInterfaceObjLock() is so 0.9.X release-y. Michal
On 05/19/2017 11:29 AM, Michal Privoznik wrote:
On 04/26/2017 12:36 AM, John Ferlan wrote:
Create/use a helper to perform the object allocation
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1cc5c92..4463653 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; +} + +
Any reason why virInterfaceObj can't actually be an virObject? virInterfaceObjLock() is so 0.9.X release-y.
Michal
I thought I tried that once - one large leap for mankind, but was asked to show all the tiny steps it took me to get there ;-) Also I didn't want the "overhead" of converting it to a virObject only to convert it later to the newer object. I mean I could now, but I have this goal of making all these driver objects use the same object around the same time. Some convert more easily since they already use virObject - others are a bit more effort. Still even if I convert it to a virObject now, that's not going to be done in "this" patch... John
On 05/20/2017 01:26 PM, John Ferlan wrote:
On 05/19/2017 11:29 AM, Michal Privoznik wrote:
On 04/26/2017 12:36 AM, John Ferlan wrote:
Create/use a helper to perform the object allocation
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1cc5c92..4463653 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; +} + +
Any reason why virInterfaceObj can't actually be an virObject? virInterfaceObjLock() is so 0.9.X release-y.
Michal
I thought I tried that once - one large leap for mankind, but was asked to show all the tiny steps it took me to get there ;-)
Right.
Also I didn't want the "overhead" of converting it to a virObject only to convert it later to the newer object. I mean I could now, but I have this goal of making all these driver objects use the same object around the same time. Some convert more easily since they already use virObject - others are a bit more effort.
Still even if I convert it to a virObject now, that's not going to be done in "this" patch...
Fair enough. It's just that whenever I see virSomethinObj I expect it to be derived from virObject. Thus I can use virObject APIs. And as for the "overhead" - I think that it'll be goot if all the objects that you will make use the new "listable" object (or whatever it is going to be called) already have common ground => are virObject already (or something derived from it). At least that's how I view these patches. What do you think? Here's the deal, I'll give you ACK on this and you'll send 9/8 to convert this to actual virObject? Michal
Also I didn't want the "overhead" of converting it to a virObject only to convert it later to the newer object. I mean I could now, but I have this goal of making all these driver objects use the same object around the same time. Some convert more easily since they already use virObject - others are a bit more effort.
Still even if I convert it to a virObject now, that's not going to be done in "this" patch...
Fair enough. It's just that whenever I see virSomethinObj I expect it to be derived from virObject. Thus I can use virObject APIs. And as for the "overhead" - I think that it'll be goot if all the objects that you will make use the new "listable" object (or whatever it is going to be called) already have common ground => are virObject already (or something derived from it). At least that's how I view these patches. What do you think? Here's the deal, I'll give you ACK on this and you'll send 9/8 to convert this to actual virObject?
I understand your point, but it's not like I invented virInterfaceObjPtr during this series either... Guess I didn't see the value in creating the OnceInit/Dispose stuff only to see it removed when the "listable" object comes along and makes it unnecessary, but I'll send 2 more patches shortly. John FWIW: A listable object would have both @active and @def, thus rendering the local object obsolete because of this code in virClassNew: } else if (parent && objectSize <= parent->objectSize) { virReportInvalidArg(objectSize, _("object size %zu of %s is smaller than parent class %zu"), objectSize, name, parent->objectSize); return NULL; } There'd be no elements in virInterfaceObj other than the @parent, so there's no use for OnceInit and friends. Yes, I went there during development ;-)
ping? NB: In the virInterfaceObjList patch, I've added a VIR_FREE(interfaces->objs); to virInterfaceObjListFree. It should be obvious why... Tks - John On 04/25/2017 06:36 PM, John Ferlan wrote:
Sensing a theme lately yet?
More adjustments in preparation for having virobject code handle the bulk of the object management code.
Far less in this series than the other two... The Clone code was a bit unique and required more of a rewrite than a refigure since now we have two allocated lists rather than two static lists.
John Ferlan (8): 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
src/conf/virinterfaceobj.c | 232 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 65 ++++++------- src/libvirt_private.syms | 15 +-- src/test/test_driver.c | 143 ++++++++++++++-------------- 4 files changed, 262 insertions(+), 193 deletions(-)
ping^2? Tks - John On 05/09/2017 12:13 PM, John Ferlan wrote:
ping?
NB: In the virInterfaceObjList patch, I've added a VIR_FREE(interfaces->objs); to virInterfaceObjListFree. It should be obvious why...
Tks -
John
On 04/25/2017 06:36 PM, John Ferlan wrote:
Sensing a theme lately yet?
More adjustments in preparation for having virobject code handle the bulk of the object management code.
Far less in this series than the other two... The Clone code was a bit unique and required more of a rewrite than a refigure since now we have two allocated lists rather than two static lists.
John Ferlan (8): 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
src/conf/virinterfaceobj.c | 232 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 65 ++++++------- src/libvirt_private.syms | 15 +-- src/test/test_driver.c | 143 ++++++++++++++-------------- 4 files changed, 262 insertions(+), 193 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/26/2017 12:36 AM, John Ferlan wrote:
Sensing a theme lately yet?
More adjustments in preparation for having virobject code handle the bulk of the object management code.
Far less in this series than the other two... The Clone code was a bit unique and required more of a rewrite than a refigure since now we have two allocated lists rather than two static lists.
John Ferlan (8): 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
src/conf/virinterfaceobj.c | 232 +++++++++++++++++++++++++++++---------------- src/conf/virinterfaceobj.h | 65 ++++++------- src/libvirt_private.syms | 15 +-- src/test/test_driver.c | 143 ++++++++++++++-------------- 4 files changed, 262 insertions(+), 193 deletions(-)
I'm tempted to ACK this, however, patch 8/8 stopped me. I think that if we are doing this, we can finally turn virInterfaceObj into virObject. ACK to the rest though. Michal
For now it'll just call the virInterfaceObjUnlock, but a future adjustment will do something different. The virInterfaceObjUnlock is now private to virinterfaceobj.c with a short term forward reference. Additionally, make virInterfaceObjLock private since it's only used in virinterfaceobj anyway. For now this will involved creating a forward reference, but this will go away soon too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 17 +++++++++++++++-- src/conf/virinterfaceobj.h | 9 +++------ src/libvirt_private.syms | 3 +-- src/test/test_driver.c | 15 +++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 1e3f25c..8f839b3 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -44,6 +44,9 @@ struct _virInterfaceObjList { virInterfaceObjPtr *objs; }; +static void +virInterfaceObjLock(virInterfaceObjPtr obj); + /* virInterfaceObj manipulation */ static virInterfaceObjPtr @@ -67,14 +70,14 @@ virInterfaceObjNew(void) } -void +static void virInterfaceObjLock(virInterfaceObjPtr obj) { virMutexLock(&obj->lock); } -void +static void virInterfaceObjUnlock(virInterfaceObjPtr obj) { virMutexUnlock(&obj->lock); @@ -82,6 +85,16 @@ virInterfaceObjUnlock(virInterfaceObjPtr obj) void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) +{ + if (!*obj) + return; + + virInterfaceObjUnlock(*obj); +} + + +void virInterfaceObjFree(virInterfaceObjPtr obj) { if (!obj) diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 3934e63..2b9e1b2 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -28,6 +28,9 @@ typedef virInterfaceObj *virInterfaceObjPtr; typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr; +void +virInterfaceObjEndAPI(virInterfaceObjPtr *obj); + virInterfaceDefPtr virInterfaceObjGetDef(virInterfaceObjPtr obj); @@ -68,12 +71,6 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj); -void -virInterfaceObjLock(virInterfaceObjPtr obj); - -void -virInterfaceObjUnlock(virInterfaceObjPtr obj); - typedef bool (*virInterfaceObjListFilter)(virConnectPtr conn, virInterfaceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa6f351..364b32e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -910,6 +910,7 @@ virDomainObjListRename; # conf/virinterfaceobj.h +virInterfaceObjEndAPI; virInterfaceObjGetDef; virInterfaceObjIsActive; virInterfaceObjListAssignDef; @@ -921,9 +922,7 @@ virInterfaceObjListGetNames; virInterfaceObjListNew; virInterfaceObjListNumOfInterfaces; virInterfaceObjListRemove; -virInterfaceObjLock; virInterfaceObjSetActive; -virInterfaceObjUnlock; # conf/virnetworkobj.h diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ac16f4f..fb95319 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1027,7 +1027,7 @@ testParseInterfaces(testDriverPtr privconn, } virInterfaceObjSetActive(obj, true); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); } ret = 0; @@ -3718,7 +3718,7 @@ testInterfaceLookupByName(virConnectPtr conn, ret = virGetInterface(conn, def->name, def->mac); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3769,7 +3769,7 @@ testInterfaceIsActive(virInterfacePtr iface) ret = virInterfaceObjIsActive(obj); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3881,7 +3881,7 @@ testInterfaceGetXMLDesc(virInterfacePtr iface, ret = virInterfaceDefFormat(def); - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3912,8 +3912,7 @@ testInterfaceDefineXML(virConnectPtr conn, cleanup: virInterfaceDefFree(def); - if (obj) - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); testDriverUnlock(privconn); return ret; } @@ -3956,7 +3955,7 @@ testInterfaceCreate(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } @@ -3983,7 +3982,7 @@ testInterfaceDestroy(virInterfacePtr iface, ret = 0; cleanup: - virInterfaceObjUnlock(obj); + virInterfaceObjEndAPI(&obj); return ret; } -- 2.9.3
Now that we have a bit more control, let's convert our object into a lockable object and let that magic handle the create and lock/unlock. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 - src/conf/virinterfaceobj.c | 110 ++++++++++++++++++++++----------------------- 2 files changed, 53 insertions(+), 58 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 5077857..4aac3bc 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -44,7 +44,6 @@ src/conf/storage_adapter_conf.c src/conf/storage_conf.c src/conf/virchrdev.c src/conf/virdomainobjlist.c -src/conf/virinterfaceobj.c src/conf/virnetworkobj.c src/conf/virnodedeviceobj.c src/conf/virnwfilterobj.c diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 8f839b3..51c3c82 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { - virMutex lock; + virObjectLockable parent; bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ @@ -44,65 +44,62 @@ struct _virInterfaceObjList { virInterfaceObjPtr *objs; }; -static void -virInterfaceObjLock(virInterfaceObjPtr obj); - /* virInterfaceObj manipulation */ -static virInterfaceObjPtr -virInterfaceObjNew(void) -{ - virInterfaceObjPtr obj; +static virClassPtr virInterfaceObjClass; +static void virInterfaceObjDispose(void *obj); - if (VIR_ALLOC(obj) < 0) - return NULL; +static int +virInterfaceObjOnceInit(void) +{ + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + "virInterfaceObj", + sizeof(virInterfaceObj), + virInterfaceObjDispose))) + return -1; - if (virMutexInit(&obj->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(obj); - return NULL; - } + return 0; +} - virInterfaceObjLock(obj); - return obj; -} +VIR_ONCE_GLOBAL_INIT(virInterfaceObj) static void -virInterfaceObjLock(virInterfaceObjPtr obj) +virInterfaceObjDispose(void *opaque) { - virMutexLock(&obj->lock); + virInterfaceObjPtr obj = opaque; + + virInterfaceDefFree(obj->def); } -static void -virInterfaceObjUnlock(virInterfaceObjPtr obj) +static virInterfaceObjPtr +virInterfaceObjNew(void) { - virMutexUnlock(&obj->lock); -} + virInterfaceObjPtr obj; + if (virInterfaceObjInitialize() < 0) + return NULL; -void -virInterfaceObjEndAPI(virInterfaceObjPtr *obj) -{ - if (!*obj) - return; + if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + return NULL; - virInterfaceObjUnlock(*obj); + virObjectLock(obj); + + return obj; } void -virInterfaceObjFree(virInterfaceObjPtr obj) +virInterfaceObjEndAPI(virInterfaceObjPtr *obj) { - if (!obj) + if (!*obj) return; - virInterfaceDefFree(obj->def); - virMutexDestroy(&obj->lock); - VIR_FREE(obj); + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; } @@ -153,18 +150,18 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STRCASEEQ(def->mac, mac)) { if (matchct < maxmatches) { if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto error; } matchct++; } } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return matchct; @@ -186,11 +183,11 @@ virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (STREQ(def->name, name)) - return obj; - virInterfaceObjUnlock(obj); + return virObjectRef(obj); + virObjectUnlock(obj); } return NULL; @@ -203,7 +200,7 @@ virInterfaceObjListFree(virInterfaceObjListPtr interfaces) size_t i; for (i = 0; i < interfaces->count; i++) - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_FREE(interfaces->objs); VIR_FREE(interfaces); } @@ -240,7 +237,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) VIR_FREE(xml); if (!(obj = virInterfaceObjListAssignDef(dest, backup))) goto error; - virInterfaceObjUnlock(obj); /* locked by virInterfaceObjListAssignDef */ + virInterfaceObjEndAPI(&obj); } return dest; @@ -269,13 +266,12 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { - virInterfaceObjUnlock(obj); - virInterfaceObjFree(obj); + virInterfaceObjEndAPI(&obj); return NULL; } obj->def = def; - return obj; + return virObjectRef(obj); } @@ -286,17 +282,17 @@ virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, { size_t i; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); for (i = 0; i < interfaces->count; i++) { - virInterfaceObjLock(interfaces->objs[i]); + virObjectLock(interfaces->objs[i]); if (interfaces->objs[i] == obj) { - virInterfaceObjUnlock(interfaces->objs[i]); - virInterfaceObjFree(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); + virObjectUnref(interfaces->objs[i]); VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } - virInterfaceObjUnlock(interfaces->objs[i]); + virObjectUnlock(interfaces->objs[i]); } } @@ -310,10 +306,10 @@ virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, for (i = 0; (i < interfaces->count); i++) { virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceObjLock(obj); + virObjectLock(obj); if (wantActive == virInterfaceObjIsActive(obj)) ninterfaces++; - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return ninterfaces; @@ -333,16 +329,16 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj = interfaces->objs[i]; virInterfaceDefPtr def; - virInterfaceObjLock(obj); + virObjectLock(obj); def = obj->def; if (wantActive == virInterfaceObjIsActive(obj)) { if (VIR_STRDUP(names[nnames], def->name) < 0) { - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); goto failure; } nnames++; } - virInterfaceObjUnlock(obj); + virObjectUnlock(obj); } return nnames; -- 2.9.3
Rather than pass by value, let's pass by reference since the object ends up "owning" the XML definition, let's make that ownership a bit more real. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Since 09 and 10 weren't reviewed yet, I'll add one more. src/conf/virinterfaceobj.c | 12 +++++++----- src/conf/virinterfaceobj.h | 2 +- src/test/test_driver.c | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 51c3c82..f7352d2 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -235,8 +235,10 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) } VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) + if (!(obj = virInterfaceObjListAssignDef(dest, &backup))) { + virInterfaceDefFree(backup); goto error; + } virInterfaceObjEndAPI(&obj); } @@ -250,13 +252,13 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces) virInterfaceObjPtr virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def) + virInterfaceDefPtr *def) { virInterfaceObjPtr obj; - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByName(interfaces, (*def)->name))) { virInterfaceDefFree(obj->def); - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return obj; } @@ -270,7 +272,7 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return NULL; } - obj->def = def; + VIR_STEAL_PTR(obj->def, *def); return virObjectRef(obj); } diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h index 2b9e1b2..0000ee9 100644 --- a/src/conf/virinterfaceobj.h +++ b/src/conf/virinterfaceobj.h @@ -65,7 +65,7 @@ virInterfaceObjListClone(virInterfaceObjListPtr interfaces); virInterfaceObjPtr virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, - virInterfaceDefPtr def); + virInterfaceDefPtr *def); void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index fb95319..4b4a782 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1021,7 +1021,7 @@ testParseInterfaces(testDriverPtr privconn, if (!def) goto error; - if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) { + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, &def))) { virInterfaceDefFree(def); goto error; } @@ -3903,9 +3903,8 @@ testInterfaceDefineXML(virConnectPtr conn, if (!(def = virInterfaceDefParseString(xmlStr))) goto cleanup; - if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, &def))) goto cleanup; - def = NULL; objdef = virInterfaceObjGetDef(obj); ret = virGetInterface(conn, objdef->name, objdef->mac); -- 2.9.4
participants (2)
-
John Ferlan -
Michal Privoznik