Rather than use VIR_WARN, let's be a bit more forceful and direct using VIR_ERROR so that the "average consumer" may actually be more concerned that something is wrong even though we still continue to operate. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virobject.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index cfa821c..38db692 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -49,17 +49,17 @@ struct _virClass { #define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)) -#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ +#define VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ if (VIR_OBJECT_NOTVALID(obj)) { \ if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ + VIR_ERROR(_("Object cannot be NULL")); \ else \ - VIR_WARN("Object %p has a bad magic number %X", \ + VIR_ERROR(_("Object %p has a bad magic number %X"), \ obj, obj->u.s.magic); \ } else { \ - VIR_WARN("Object %p (%s) is not a %s instance", \ + VIR_ERROR(_("Object %p (%s) is not a %s instance"), \ anyobj, obj->klass->name, #objclass); \ } \ } while (0) @@ -396,7 +396,7 @@ virObjectGetLockableObj(void *anyobj) if (virObjectIsClass(anyobj, virObjectLockableClass)) return anyobj; - VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockable); + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLockable); return NULL; } @@ -407,7 +407,7 @@ virObjectGetRWLockableObj(void *anyobj) if (virObjectIsClass(anyobj, virObjectRWLockableClass)) return anyobj; - VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectRWLockable); + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectRWLockable); return NULL; } -- 2.9.4

Using the virObjectRWLockable class as the base with the concepts from virObjectLookupKeys of either 1 or 2 lookup keys possible, create the virObjectLookupHash to manage a common mechanism to store objects for driver/vir*obj.c via self locking hash tables using the RW locking APIs. Each hash table will have: objsKey1 -> Hash table for the LookupKeys->key1 and may also have: objsKey2 -> Hash table for the LookupKeys->key2 A secondary benefit of self locking hash tables is each driver then does not have to keep a higher level driver lock for interactions with the object storage since the object itself can manage locking as needed. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 23 ++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 082a0ba..66cf865 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2318,6 +2318,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupHash; virClassForObjectLookupKeys; virClassForObjectRWLockable; virClassIsDerivedFrom; @@ -2330,6 +2331,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashNew; virObjectLookupKeysNew; virObjectNew; virObjectRef; diff --git a/src/util/virobject.c b/src/util/virobject.c index aec10eb..f677b5f 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -69,10 +69,12 @@ static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectRWLockableClass; static virClassPtr virObjectLookupKeysClass; +static virClassPtr virObjectLookupHashClass; static void virObjectLockableDispose(void *anyobj); static void virObjectRWLockableDispose(void *anyobj); static void virObjectLookupKeysDispose(void *anyobj); +static void virObjectLookupHashDispose(void *anyobj); static int virObjectOnceInit(void) @@ -101,6 +103,12 @@ virObjectOnceInit(void) virObjectLookupKeysDispose))) return -1; + if (!(virObjectLookupHashClass = virClassNew(virObjectRWLockableClass, + "virObjectLookupHash", + sizeof(virObjectLookupHash), + virObjectLookupHashDispose))) + return -1; + return 0; } @@ -168,6 +176,21 @@ virClassForObjectLookupKeys(void) /** + * virClassForObjectLookupHash: + * + * Returns the class instance for the virObjectLookupHash type + */ +virClassPtr +virClassForObjectLookupHash(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + return virObjectLookupHashClass; +} + + +/** * virClassNew: * @parent: the parent class * @name: the class name @@ -413,6 +436,63 @@ virObjectLookupKeysDispose(void *anyobj) /** + * virObjectLookupHashNew: + * @klass: the klass to check + * @tableElemsStart: initial size of each hash table + * @createBoth: boolean to determine how many hash tables to create + * + * Create a new poolable hash table object for storing either 1 or 2 + * hash tables capable of storing virObjectLookupKeys objects. This + * object will use the RWLockable objects in order to allow for concurrent + * table reads by multiple threads looking to return lists of data. + * + * Returns: New object on success, NULL on failure w/ error message set + */ +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart, + bool createBoth) +{ + virObjectLookupHashPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupHash())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupHash"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectRWLockableNew(klass))) + return NULL; + + if (!(obj->objsKey1 = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + if (createBoth && + !(obj->objsKey2 = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupHashDispose(void *anyobj) +{ + virObjectLookupHashPtr obj = anyobj; + + virHashFree(obj->objsKey1); + virHashFree(obj->objsKey2); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -490,7 +570,8 @@ virObjectGetLockableObj(void *anyobj) static virObjectRWLockablePtr virObjectGetRWLockableObj(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectRWLockableClass)) + if (virObjectIsClass(anyobj, virObjectRWLockableClass) || + virObjectIsClass(anyobj, virObjectLookupHashClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectRWLockable); diff --git a/src/util/virobject.h b/src/util/virobject.h index 0f7d5ca..a5c03be 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -23,6 +23,7 @@ # define __VIR_OBJECT_H__ # include "internal.h" +# include "virhash.h" # include "virthread.h" typedef struct _virClass virClass; @@ -40,6 +41,9 @@ typedef virObjectRWLockable *virObjectRWLockablePtr; typedef struct _virObjectLookupKeys virObjectLookupKeys; typedef virObjectLookupKeys *virObjectLookupKeysPtr; +typedef struct _virObjectLookupHash virObjectLookupHash; +typedef virObjectLookupHash *virObjectLookupHashPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -77,11 +81,24 @@ struct _virObjectLookupKeys { char *key2; }; +struct _virObjectLookupHash { + virObjectRWLockable parent; + + /* key1 string -> object mapping for O(1), + * lockless lookup-by-key1 */ + virHashTable *objsKey1; + + /* key2 string -> object mapping for O(1), + * lockless lookup-by-key2 */ + virHashTable *objsKey2; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectRWLockable(void); virClassPtr virClassForObjectLookupKeys(void); +virClassPtr virClassForObjectLookupHash(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -138,6 +155,12 @@ virObjectLookupKeysNew(virClassPtr klass, const char *key2) ATTRIBUTE_NONNULL(1); +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart, + bool createBoth) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4

Add a pair of API's to add/remove the LookupKeys object to/from the LookupHash object. The caller must check return status and handle failure properly for the Add. The Remove API callers are all void functions, so only report the problem and ignore the attempt to remove if for some reason the passed @tableobj is not as expected. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 10 ++++++ 3 files changed, 104 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9eb0589..c6d22d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2331,7 +2331,9 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashAdd; virObjectLookupHashNew; +virObjectLookupHashRemove; virObjectLookupKeysIsActive; virObjectLookupKeysNew; virObjectLookupKeysSetActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 657597f..59c8ac6 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -858,3 +858,95 @@ virObjectLookupKeysSetActive(void *anyobj, obj->active = active; } + + +static virObjectLookupHashPtr +virObjectGetLookupHashObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupHashClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLookupHashClass); + + return NULL; +} + + +/** + * virObjectLookupHashAdd: + * @anyobj: LookupHash object + * @obj: The LookupKeys object to insert in the hash table(s) + * + * Insert @obj into the hash tables found in @anyobj. Assumes that the + * caller has determined that the key1 and possibly key2 do not already + * exist in their respective hash table to be inserted. + * + * Returns 0 on success, -1 on failure. + */ +int +virObjectLookupHashAdd(void *anyobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return -1; + + if (obj->key2 && !hashObj->objsKey2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hashObj=%p has one table, but two keys from obj=%p"), + hashObj, obj); + return -1; + } + + if (virHashAddEntry(hashObj->objsKey1, obj->key1, obj) < 0) + return -1; + virObjectRef(obj); + + if (obj->key2) { + if (virHashAddEntry(hashObj->objsKey2, obj->key2, obj) < 0) { + virHashRemoveEntry(hashObj->objsKey1, obj->key1); + return -1; + } + virObjectRef(obj); + } + + return 0; +} + + +/** + * virObjectLookupHashRemove: + * @anyobj: LookupHash object + * @obj: The LookupKeys object to remove from the hash table(s) + * + * Remove @obj from the hash tables found in @anyobj. The common + * function to remove an object from a hash table will also cause + * the virObjectUnref to be called via virObjectFreeHashData since + * the virHashCreate used that as the Free object element argument. + * + * Even though this is a void, report the error for a bad @anyobj. + */ +void +virObjectLookupHashRemove(void *anyobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj; + + if (!obj) + return; + + if (!(hashObj = virObjectGetLookupHashObj(anyobj))) + return; + + virObjectRef(obj); + virObjectUnlock(obj); + virObjectRWLockWrite(hashObj); + virObjectLock(obj); + virHashRemoveEntry(hashObj->objsKey1, obj->key1); + if (obj->key2) + virHashRemoveEntry(hashObj->objsKey2, obj->key2); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 0d3b90b..bb02781 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -199,4 +199,14 @@ virObjectLookupKeysSetActive(void *anyobj, bool active) ATTRIBUTE_NONNULL(1); +int +virObjectLookupHashAdd(void *anyobj, + virObjectLookupKeysPtr obj) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void +virObjectLookupHashRemove(void *anyobj, + virObjectLookupKeysPtr obj) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_OBJECT_H */ -- 2.9.4

Introduce an API to use the virHashForEach API to go through each element of the LookupHash calling a callback with a LookupKey object entry from the LookupHash in order for it to be processed. Create a common structure _virObjectLookupHashForEachData to define the various pieces of data needed by the callback consumers. Fields described in the API function description section. Upon successful completion, the data.nElems is returned to the caller and possibly various fields within the structure filled in depending on the callers need as follows: vir{Object}NumOf{Elem} vir{Object}Get{Key} => Callback function can be combined since generally all that the NumOf cares about is counts, but the Get function also adds some {Key} to a returned list of @names. vir{Object}Export => When @maxElems == -1, will cause allocation into data->elems of an array sized by the hash table size which will be used by the callback function to store addresses of objects specific to each {Object} that are typically associated with the virConnectListAll* API's from the virGet{Object}() calls. The @flags argument is typically used for additional filtering via vir{Object}Match or vir{Object}Filter APIs found in various vir*obj modules. Upon return from the call, the calling function must then move the data->elems into the returned structure. It is up to the callback function to set @error in the event of an error during processing resulting in calling the VIR_FREE() for each of the @nElems already in the array. When an error occurs, any collection done by the callback functions into the @elems array is discarded. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virobject.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 21 ++++++++++++++ 3 files changed, 96 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b6ab173..3978106 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2334,6 +2334,7 @@ virObjectLockableNew; virObjectLookupHashAdd; virObjectLookupHashFind; virObjectLookupHashFindLocked; +virObjectLookupHashForEach; virObjectLookupHashNew; virObjectLookupHashRemove; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 7dbfd1b..a76f1cb 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1019,3 +1019,77 @@ virObjectLookupHashFind(void *anyobj, return obj; } + + +/** + * virObjectLookupHashForEach + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * For each element of the objsKey1 hash table make a call into the + * callback routine to handle its task. Even if there were two hash + * tables all the objects exist in both, so it's only necessary to + * run through one of them. + * + * NB: + * struct _virObjectLookupHashForEachData { + * virConnectPtr conn; -> Connect ptr for @filter APIs + * void *opaque; -> Opaque data as determined by caller + * void *filter; -> A pointer to function for ACL calls + * bool wantActive; -> Filter active objs + * bool error; -> Set by callback functions for error + * const char *matchStr; -> Filter for specific string in many objs + * unsigned int flags; -> @flags argument to for Export calls + * int nElems; -> # of elements found and passing filters + * void **elems; -> array of elements + * int maxElems; -> maximum # of elements to collect + * Use -1 to allocate array of N table sized + * elements to use for Export functions + * Use -2 for NumOf functions to avoid the + * allocation, but allow sharing with the + * GetNames type functions + * }; + * + * Returns number of elements found on success, -1 on failure + */ +int +virObjectLookupHashForEach(void *anyobj, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return -1; + + if (data->maxElems == -1) { + if (VIR_ALLOC_N(data->elems, virHashSize(hashObj->objsKey1) + 1) < 0) + return -1; + } + + virObjectRWLockRead(hashObj); + virHashForEach(hashObj->objsKey1, callback, data); + virObjectRWUnlock(hashObj); + + if (data->error) + goto error; + + if (data->maxElems == -1) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data->elems, data->nElems + 1)); + } + + return data->nElems; + + error: + if (data->elems) { + if (data->maxElems == -1) { + virObjectListFree(data->elems); + } else { + while (--data->nElems) + VIR_FREE(data->elems[data->nElems]); + } + } + return -1; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index dc668ca..ddebf6c 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -219,4 +219,25 @@ virObjectLookupHashFind(void *anyobj, const char *key) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +typedef struct _virObjectLookupHashForEachData virObjectLookupHashForEachData; +typedef virObjectLookupHashForEachData *virObjectLookupHashForEachDataPtr; +struct _virObjectLookupHashForEachData { + virConnectPtr conn; + void *opaque; + void *filter; + bool wantActive; + bool error; + const char *matchStr; + unsigned int flags; + int nElems; + void **elems; + int maxElems; +}; + +int +virObjectLookupHashForEach(void *anyobj, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_OBJECT_H */ -- 2.9.4

Use the virObjectLookupKeys in _virNodeDeviceObj and use the virObjectLookupHash in _virNodeDeviceObjList. Convert the code to use the LookupHash object and APIs rather than code within this module that uses virHash* calls. Since the _virNodeDeviceObjList only now contains the @parent object, the virClassNew must be removed from OnceInit because instantiation would fail since the object size would be the same as the parent object size. Usage of HashLookup{Find|Search} APIs returns a locked/reffed object so remove the virObjectLock after FindBy*Locked calls. The only function requiring taking a lock is the Add function since it needs to be synchronized in such a way to avoid multiple threads attempting to add the same named node device at the same time. Also change the virNodeDeviceObjListFilter to follow convention of other drivers of virNodeDeviceObjListACLFilter. The NumOfDevicesCallback and GetNamesCallback can use the same callback function with the only difference being the filling in of the @names array from the passed @data structure if it exists. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnodedeviceobj.c | 286 +++++++++++++------------------------------- 1 file changed, 80 insertions(+), 206 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b0dcee1..7f025b6 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -34,41 +34,28 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); struct _virNodeDeviceObj { - virObjectLockable parent; + virObjectLookupKeys parent; virNodeDeviceDefPtr def; /* device definition */ }; struct _virNodeDeviceObjList { - virObjectLockable parent; - - /* name string -> virNodeDeviceObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs; - + virObjectLookupHash parent; }; static virClassPtr virNodeDeviceObjClass; -static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); -static void virNodeDeviceObjListDispose(void *opaque); static int virNodeDeviceObjOnceInit(void) { - if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLockable(), + if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLookupKeys(), "virNodeDeviceObj", sizeof(virNodeDeviceObj), virNodeDeviceObjDispose))) return -1; - if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), - "virNodeDeviceObjList", - sizeof(virNodeDeviceObjList), - virNodeDeviceObjListDispose))) - return -1; - return 0; } @@ -85,14 +72,14 @@ virNodeDeviceObjDispose(void *opaque) static virNodeDeviceObjPtr -virNodeDeviceObjNew(void) +virNodeDeviceObjNew(const char *name) { virNodeDeviceObjPtr obj; if (virNodeDeviceObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virNodeDeviceObjClass))) + if (!(obj = virObjectLookupKeysNew(virNodeDeviceObjClass, name, NULL))) return NULL; virObjectLock(obj); @@ -229,17 +216,9 @@ virNodeDeviceObjListSearch(virNodeDeviceObjListPtr devs, virHashSearcher callback, const void *data) { - virNodeDeviceObjPtr obj; - - virObjectLock(devs); - obj = virHashSearch(devs->objs, callback, data, NULL); - virObjectRef(obj); - virObjectUnlock(devs); - - if (obj) - virObjectLock(obj); - - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashSearch(devs, callback, + (void *)data); + return (virNodeDeviceObjPtr) obj; } @@ -274,7 +253,8 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, const char *name) { - return virObjectRef(virHashLookup(devs->objs, name)); + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(devs, name); + return (virNodeDeviceObjPtr) obj; } @@ -282,15 +262,8 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - virNodeDeviceObjPtr obj; - - virObjectLock(devs); - obj = virNodeDeviceObjListFindByNameLocked(devs, name); - virObjectUnlock(devs); - if (obj) - virObjectLock(obj); - - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(devs, name); + return (virNodeDeviceObjPtr) obj; } @@ -445,32 +418,10 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, } -static void -virNodeDeviceObjListDispose(void *obj) -{ - virNodeDeviceObjListPtr devs = obj; - - virHashFree(devs->objs); -} - - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void) { - virNodeDeviceObjListPtr devs; - - if (virNodeDeviceObjInitialize() < 0) - return NULL; - - if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) - return NULL; - - if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { - virObjectUnref(devs); - return NULL; - } - - return devs; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 50, false); } @@ -486,29 +437,31 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; + virNodeDeviceObjPtr ret = NULL; - virObjectLock(devs); + virObjectRWLockWrite(devs); if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) { - virObjectLock(obj); virNodeDeviceDefFree(obj->def); obj->def = def; } else { - if (!(obj = virNodeDeviceObjNew())) + if (!(obj = virNodeDeviceObjNew(def->name))) goto cleanup; - if (virHashAddEntry(devs->objs, def->name, obj) < 0) { - virNodeDeviceObjEndAPI(&obj); + if (virObjectLookupHashAdd(devs, (virObjectLookupKeysPtr)obj) < 0) goto cleanup; - } obj->def = def; virObjectRef(obj); } + ret = obj; + obj = NULL; + cleanup: - virObjectUnlock(devs); - return obj; + virNodeDeviceObjEndAPI(&obj); + virObjectRWUnlock(devs); + return ret; } @@ -516,20 +469,7 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - virNodeDeviceDefPtr def; - - if (!obj) - return; - def = obj->def; - - virObjectRef(obj); - virObjectUnlock(obj); - virObjectLock(devs); - virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); - virObjectUnlock(obj); - virObjectUnref(obj); - virObjectUnlock(devs); + virObjectLookupHashRemove(devs, (virObjectLookupKeysPtr)obj); } @@ -730,29 +670,33 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, } -struct virNodeDeviceCountData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - const char *matchstr; - int count; -}; - static int -virNodeDeviceObjListNumOfDevicesCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +virNodeDeviceObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { virNodeDeviceObjPtr obj = payload; virNodeDeviceDefPtr def; - struct virNodeDeviceCountData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; virNodeDeviceObjListFilter filter = data->filter; + char **names = (char **)data->elems; + + if (data->error) + return 0; virObjectLock(obj); def = obj->def; + if ((!filter || filter(data->conn, def)) && - (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) - data->count++; + (!data->matchStr || virNodeDeviceObjHasCap(obj, data->matchStr))) { + if (names && VIR_STRDUP(names[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nElems++; + } + cleanup: virObjectUnlock(obj); return 0; } @@ -764,55 +708,11 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, const char *cap, virNodeDeviceObjListFilter filter) { - struct virNodeDeviceCountData data = { - .conn = conn, .filter = filter, .matchstr = cap, .count = 0 }; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = NULL, .maxElems = -2 }; - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); - virObjectUnlock(devs); - - return data.count; -} - - -struct virNodeDeviceGetNamesData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - const char *matchstr; - int nnames; - char **names; - int maxnames; - bool error; -}; - -static int -virNodeDeviceObjListGetNamesCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) -{ - virNodeDeviceObjPtr obj = payload; - virNodeDeviceDefPtr def; - struct virNodeDeviceGetNamesData *data = opaque; - virNodeDeviceObjListFilter filter = data->filter; - - if (data->error) - return 0; - - virObjectLock(obj); - def = obj->def; - - if ((!filter || filter(data->conn, def)) && - (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) { - if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) { - data->error = true; - goto cleanup; - } - data->nnames++; - } - - cleanup: - virObjectUnlock(obj); - return 0; + return virObjectLookupHashForEach(devs, virNodeDeviceObjListGetHelper, &data); } @@ -824,23 +724,11 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, char **const names, int maxnames) { - struct virNodeDeviceGetNamesData data = { - .conn = conn, .filter = filter, .matchstr = cap, .names = names, - .nnames = 0, .maxnames = maxnames, .error = false }; - - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); - virObjectUnlock(devs); - - if (data.error) - goto error; - - return data.nnames; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = (void **)names, .maxElems = maxnames }; - error: - while (--data.nnames) - VIR_FREE(data.names[data.nnames]); - return -1; + return virObjectLookupHashForEach(devs, virNodeDeviceObjListGetHelper, &data); } @@ -876,15 +764,6 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj, #undef MATCH -struct virNodeDeviceObjListExportData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - unsigned int flags; - virNodeDevicePtr *devices; - int ndevices; - bool error; -}; - static int virNodeDeviceObjListExportCallback(void *payload, const void *name ATTRIBUTE_UNUSED, @@ -892,8 +771,10 @@ virNodeDeviceObjListExportCallback(void *payload, { virNodeDeviceObjPtr obj = payload; virNodeDeviceDefPtr def; - struct virNodeDeviceObjListExportData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + virNodeDeviceObjListFilter filter = data->filter; virNodeDevicePtr device = NULL; + virNodeDevicePtr *devices = (virNodeDevicePtr *)data->elems; if (data->error) return 0; @@ -901,19 +782,24 @@ virNodeDeviceObjListExportCallback(void *payload, virObjectLock(obj); def = obj->def; - if ((!data->filter || data->filter(data->conn, def)) && - virNodeDeviceMatch(obj, data->flags)) { - if (data->devices) { - if (!(device = virGetNodeDevice(data->conn, def->name)) || - VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - data->error = true; - goto cleanup; - } - data->devices[data->ndevices] = device; - } - data->ndevices++; + if (filter && !filter(data->conn, def)) + goto cleanup; + + if (!virNodeDeviceMatch(obj, data->flags)) + goto cleanup; + + if (!devices) { + data->nElems++; + goto cleanup; + } + + if (!(device = virGetNodeDevice(data->conn, def->name)) || + VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + data->error = true; + goto cleanup; } + devices[data->nElems++] = device; cleanup: virObjectUnlock(obj); @@ -928,31 +814,19 @@ virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListFilter filter, unsigned int flags) { - struct virNodeDeviceObjListExportData data = { - .conn = conn, .filter = filter, .flags = flags, - .devices = NULL, .ndevices = 0, .error = false }; - - virObjectLock(devs); - if (devices && - VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) { - virObjectUnlock(devs); - return -1; - } - - virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); - virObjectUnlock(devs); + int ret; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - if (data.error) - goto cleanup; + if (devices) + data.maxElems = -1; - if (data.devices) { - ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1)); - *devices = data.devices; - } + ret = virObjectLookupHashForEach(devs, virNodeDeviceObjListExportCallback, + &data); - return data.ndevices; + if (devices) + *devices = (virNodeDevicePtr *)data.elems; - cleanup: - virObjectListFree(data.devices); - return -1; + return ret; } -- 2.9.4

A convenience API that will utilize the virHashForEach API for the LookupHash in order to create a clone/copy. Primary consumer is the interface driver which has a desire to save off a copy of its only hash table in order to possible restore it if something goes wrong during processing. The callback function's primary purpose is to copy anything within the local LookupKeys into the target. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virobject.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 9 ++++++ 3 files changed, 89 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index df3f246..c7c9762 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2332,6 +2332,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashClone; virObjectLookupHashFind; virObjectLookupHashFindLocked; virObjectLookupHashForEach; diff --git a/src/util/virobject.c b/src/util/virobject.c index 17ea4e6..0a4195d 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1167,3 +1167,82 @@ virObjectLookupHashSearch(void *anyobj, return obj; } + + +struct cloneData { + virObjectLookupHashCloneCallback callback; + virObjectLookupHashPtr dst; + bool error; +}; + +/* + * Take the provided virHashForEach element and call the @cb function + * with the input @dst hash table and the source element from the + * @src hash table in order to perform the copy - tracking success/ + * failure using the error boolean. + * + * Once there's a failure, no future copy/clone will occur. + * + * The @cb function can expect the @src hash table object to be + * locked upon entry. + * + * Returns 0 to the virHashForEach on success, -1 on failure. + */ +static int +cloneCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virObjectLookupKeysPtr obj = payload; + struct cloneData *data = opaque; + + if (data->error) + return 0; + + virObjectLock(obj); + + if (data->callback(data->dst, obj) < 0) + data->error = true; + + virObjectUnlock(obj); + + if (data->error) + return -1; + + return 0; +} + +/** + * virObjectLookupHashClone + * @srcAnyobj: source LookupHash object to clone from + * @dstAnyobj: destination LookupHash object to clone to + * @cb: callback function to handle the clone + * + * The clone function is designed to traverse each source hash element + * and call the driver specific @cb function with the element from the + * source hash table in order to clone into the destination hash table. + * + * Return 0 on success, -1 on failure + */ +int +virObjectLookupHashClone(void *srcAnyobj, + void *dstAnyobj, + virObjectLookupHashCloneCallback cb) +{ + virObjectLookupHashPtr srcHashObj = virObjectGetLookupHashObj(srcAnyobj); + virObjectLookupHashPtr dstHashObj = virObjectGetLookupHashObj(dstAnyobj); + struct cloneData data = { .callback = cb, .dst = dstHashObj, + .error = false }; + + if (!srcHashObj || !dstHashObj) + return -1; + + virObjectRWLockRead(srcHashObj); + virHashForEach(srcHashObj->objsKey1, cloneCallback, &data); + virObjectRWUnlock(srcHashObj); + + if (data.error) + return -1; + + return 0; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index d4bc3c3..b9e6311 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -252,4 +252,13 @@ virObjectLookupHashSearch(void *anyobj, void *opaque) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +typedef int (*virObjectLookupHashCloneCallback)(void *destHashTable, + void *sourceObject); +int +virObjectLookupHashClone(void *srcAnyobj, + void *dstAnyobj, + virObjectLookupHashCloneCallback cb) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + + #endif /* __VIR_OBJECT_H */ -- 2.9.4

Use the virObjectLookupKeys in _virInterfaceObj and use the virObjectLookupHash in _virInterfaceObjList. Convert the code to use the LookupHash object and APIs rather than the local forward linked list processing. Usage of HashLookup{Find|Search} API's is via a callback mechanism and returns a locked/reffed object. The Clone API will make use of the ForEach functionality in copying whatever is in one HashLookup list into the destination. The only function requiring taking a lock is the AssignDef function since it needs to be synchronized in such a way to avoid multiple threads attempting to add the same named object at the same time. The NumOfInterfaces and GetNames can use the same callback function with the only difference being the filling in of the @names array from the passed @data structure if it exists. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virinterfaceobj.c | 300 ++++++++++++++++++++++++--------------------- 1 file changed, 157 insertions(+), 143 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e993b92..f2475b8 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -33,15 +33,13 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { - virObjectLockable parent; + virObjectLookupKeys parent; - bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ }; struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; + virObjectLookupHash parent; }; /* virInterfaceObj manipulation */ @@ -52,7 +50,7 @@ static void virInterfaceObjDispose(void *obj); static int virInterfaceObjOnceInit(void) { - if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLookupKeys(), "virInterfaceObj", sizeof(virInterfaceObj), virInterfaceObjDispose))) @@ -74,14 +72,14 @@ virInterfaceObjDispose(void *opaque) static virInterfaceObjPtr -virInterfaceObjNew(void) +virInterfaceObjNew(const char *name) { virInterfaceObjPtr obj; if (virInterfaceObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + if (!(obj = virObjectLookupKeysNew(virInterfaceObjClass, name, NULL))) return NULL; virObjectLock(obj); @@ -112,7 +110,7 @@ virInterfaceObjGetDef(virInterfaceObjPtr obj) bool virInterfaceObjIsActive(virInterfaceObjPtr obj) { - return obj->active; + return virObjectLookupKeysIsActive(obj); } @@ -120,7 +118,7 @@ void virInterfaceObjSetActive(virInterfaceObjPtr obj, bool active) { - obj->active = active; + virObjectLookupKeysSetActive(obj, active); } @@ -128,11 +126,40 @@ virInterfaceObjSetActive(virInterfaceObjPtr obj, virInterfaceObjListPtr virInterfaceObjListNew(void) { - virInterfaceObjListPtr interfaces; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 10, false); +} - if (VIR_ALLOC(interfaces) < 0) - return NULL; - return interfaces; + +static int +virInterfaceObjListFindByMACStringCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + char **const matches = (char **const)data->elems; + virInterfaceDefPtr def; + int ret = -1; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; + if (STRCASEEQ(def->mac, data->matchStr)) { + if (data->nElems < data->maxElems) { + if (VIR_STRDUP(matches[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nElems++; + } + } + ret = 0; + + cleanup: + virObjectUnlock(obj); + return ret; } @@ -142,33 +169,22 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, char **const matches, int maxmatches) { - size_t i; - int matchct = 0; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (STRCASEEQ(def->mac, mac)) { - if (matchct < maxmatches) { - if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virObjectUnlock(obj); - goto error; - } - matchct++; - } - } - virObjectUnlock(obj); - } - return matchct; + virObjectLookupHashForEachData data = { + .error = false, .matchStr = mac, .nElems = 0, + .elems = (void **)matches, .maxElems = maxmatches }; + + return virObjectLookupHashForEach(interfaces, + virInterfaceObjListFindByMACStringCb, + &data); +} - error: - while (--matchct >= 0) - VIR_FREE(matches[matchct]); - return -1; +static virInterfaceObjPtr +virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces, + const char *name) +{ + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(interfaces, name); + return (virInterfaceObjPtr)obj; } @@ -176,73 +192,73 @@ virInterfaceObjPtr virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, const char *name) { - size_t i; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - - return NULL; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(interfaces, name); + return (virInterfaceObjPtr) obj; } void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) { - size_t i; + virObjectUnref(interfaces); +} + + +struct interfaceCloneData { + const char *primaryKey; + virInterfaceObjListPtr dest; + bool error; +}; + +static int +virInterfaceObjListCloneCb(void *destHashTable, + void *sourceObject) +{ + virInterfaceObjListPtr dest = destHashTable; + virInterfaceObjPtr srcObj = sourceObject; + int ret = -1; + char *xml = NULL; + virInterfaceObjPtr obj; + virInterfaceDefPtr backup = NULL; + + if (!(xml = virInterfaceDefFormat(srcObj->def))) + goto cleanup; - for (i = 0; i < interfaces->count; i++) - virObjectUnref(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - VIR_FREE(interfaces); + if (!(backup = virInterfaceDefParseString(xml))) + goto cleanup; + + if (!(obj = virInterfaceObjListAssignDef(dest, backup))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(xml); + virInterfaceDefFree(backup); + + return ret; } virInterfaceObjListPtr virInterfaceObjListClone(virInterfaceObjListPtr interfaces) { - size_t i; - unsigned int cnt; - virInterfaceObjListPtr dest; + virInterfaceObjListPtr destInterfaces = NULL; if (!interfaces) return NULL; - if (!(dest = virInterfaceObjListNew())) + if (!(destInterfaces = virInterfaceObjListNew())) return NULL; - cnt = interfaces->count; - for (i = 0; i < cnt; i++) { - virInterfaceObjPtr srcobj = interfaces->objs[i]; - virInterfaceDefPtr backup; - virInterfaceObjPtr obj; - char *xml = virInterfaceDefFormat(srcobj->def); + if (virObjectLookupHashClone(interfaces, destInterfaces, + virInterfaceObjListCloneCb) < 0) + goto cleanup; - if (!xml) - goto error; - - if (!(backup = virInterfaceDefParseString(xml))) { - VIR_FREE(xml); - goto error; - } - - VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) - goto error; - virInterfaceObjEndAPI(&obj); - } + return destInterfaces; - return dest; - - error: - virInterfaceObjListFree(dest); + cleanup: + virObjectUnref(destInterfaces); return NULL; } @@ -252,24 +268,29 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { virInterfaceObjPtr obj; + virInterfaceObjPtr ret = NULL; + + virObjectRWLockWrite(interfaces); - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + } else { + if (!(obj = virInterfaceObjNew(def->name))) + goto cleanup; - return obj; + if (virObjectLookupHashAdd(interfaces, (virObjectLookupKeysPtr)obj) < 0) + goto cleanup; + obj->def = def; } - if (!(obj = virInterfaceObjNew())) - return NULL; + ret = obj; + obj = NULL; - if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } - obj->def = def; - return virObjectRef(obj); + cleanup: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return ret; } @@ -277,20 +298,39 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i; + virObjectLookupHashRemove(interfaces, (virObjectLookupKeysPtr)obj); +} - virObjectUnlock(obj); - for (i = 0; i < interfaces->count; i++) { - virObjectLock(interfaces->objs[i]); - if (interfaces->objs[i] == obj) { - virObjectUnlock(interfaces->objs[i]); - virObjectUnref(interfaces->objs[i]); - - VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); - break; + +static int +virInterfaceObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + char **const names = (char **const)data->elems; + virInterfaceDefPtr def; + + if (data->error) + return 0; + + if (data->maxElems >= 0 && data->nElems == data->maxElems) + return 0; + + virObjectLock(obj); + def = obj->def; + if (data->wantActive == virInterfaceObjIsActive(obj)) { + if (names && VIR_STRDUP(names[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; } - virObjectUnlock(interfaces->objs[i]); - } + data->nElems++; + } + + cleanup: + virObjectUnlock(obj); + return 0; } @@ -298,18 +338,12 @@ int virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, bool wantActive) { - size_t i; - int ninterfaces = 0; - - for (i = 0; (i < interfaces->count); i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virObjectLock(obj); - if (wantActive == virInterfaceObjIsActive(obj)) - ninterfaces++; - virObjectUnlock(obj); - } + virObjectLookupHashForEachData data = { + .wantActive = wantActive, .error = false, .nElems = 0, + .elems = NULL, .maxElems = -2 }; - return ninterfaces; + return virObjectLookupHashForEach(interfaces, virInterfaceObjListGetHelper, + &data); } @@ -319,30 +353,10 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, char **const names, int maxnames) { - int nnames = 0; - size_t i; - - for (i = 0; i < interfaces->count && nnames < maxnames; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (wantActive == virInterfaceObjIsActive(obj)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); - } - - return nnames; - - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + virObjectLookupHashForEachData data = { + .wantActive = wantActive, .error = false, .nElems = 0, + .elems = (void **)names, .maxElems = maxnames }; - return -1; + return virObjectLookupHashForEach(interfaces, virInterfaceObjListGetHelper, + &data); } -- 2.9.4

A convenience API that will utilize the virHashForEach API for the LookupHash in order to remove elements from the hash table(s) that match some requirement from the callback. NB: Once elements are removed from objsKey1 - if something goes wrong in objsKey2, the best that can be done is to issue a warning and hope for the best. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virobject.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 5 +++++ 3 files changed, 42 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7c9762..47393c6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2337,6 +2337,7 @@ virObjectLookupHashFind; virObjectLookupHashFindLocked; virObjectLookupHashForEach; virObjectLookupHashNew; +virObjectLookupHashPrune; virObjectLookupHashRemove; virObjectLookupHashSearch; virObjectLookupHashSearchLocked; diff --git a/src/util/virobject.c b/src/util/virobject.c index 0a4195d..47787f7 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1246,3 +1246,39 @@ virObjectLookupHashClone(void *srcAnyobj, return 0; } + + +/** + * virObjectLookupHashPrune + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * Call the callback function from virHashRemoveSet in order to determine + * if the incoming opaque data should be removed from the hash table(s) + * for the LookupHash. If a second hash table exists and we fail to remove + * data from it + */ +void +virObjectLookupHashPrune(void *anyobj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + ssize_t cntKey1; + ssize_t cntKey2; + + if (!hashObj) + return; + + virObjectRWLockWrite(hashObj); + cntKey1 = virHashRemoveSet(hashObj->objsKey1, callback, opaque); + if (cntKey1 > 0 && hashObj->objsKey2) { + cntKey2 = virHashRemoveSet(hashObj->objsKey2, callback, opaque); + + if (cntKey2 != cntKey1) + VIR_ERROR(_("removed %zd elems from objsKey1 and %zd elems " + "from objsKey2"), cntKey1, cntKey2); + } + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index b9e6311..54eff81 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -260,5 +260,10 @@ virObjectLookupHashClone(void *srcAnyobj, virObjectLookupHashCloneCallback cb) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void +virObjectLookupHashPrune(void *anyobj, + virHashSearcher callback, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __VIR_OBJECT_H */ -- 2.9.4

Use the virObjectLookupKeys in _virNetworkObj and use the virObjectLookupHash in _virNetworkObjList. Convert the code to use the LookupHash object and APIs rather than the local code and usage of virHash* calls. Since the _virNetworkObjList only contains the @parent object the virClassNew must be removed from OnceInit because instantiation would fail since the object size would be the same as the parent object size. Usage of HashLookup{Find|Search} API's returns a locked/reffed object so need to remove virObjectLock after FindBy*Locked calls. Use the def->name as a second key to the LookupHash to make for faster lookup's by name (instead of using Search on uuid key and matching the name. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnetworkobj.c | 291 ++++++++++++-------------------------------- src/conf/virnetworkobj.h | 3 +- tests/networkxml2conftest.c | 4 +- 3 files changed, 86 insertions(+), 212 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 20f846d..d175191 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -40,11 +40,10 @@ VIR_LOG_INIT("conf.virnetworkobj"); #define INIT_CLASS_ID_BITMAP_SIZE (1<<4) struct _virNetworkObj { - virObjectLockable parent; + virObjectLookupKeys parent; pid_t dnsmasqPid; pid_t radvdPid; - bool active; bool autostart; bool persistent; @@ -61,30 +60,21 @@ struct _virNetworkObj { }; struct _virNetworkObjList { - virObjectLockable parent; - - virHashTablePtr objs; + virObjectLookupHash parent; }; static virClassPtr virNetworkObjClass; -static virClassPtr virNetworkObjListClass; static void virNetworkObjDispose(void *obj); -static void virNetworkObjListDispose(void *obj); static int virNetworkObjOnceInit(void) { - if (!(virNetworkObjClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjClass = virClassNew(virClassForObjectLookupKeys(), "virNetworkObj", sizeof(virNetworkObj), virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), - "virNetworkObjList", - sizeof(virNetworkObjList), - virNetworkObjListDispose))) - return -1; return 0; } @@ -92,14 +82,15 @@ virNetworkObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetworkObj) virNetworkObjPtr -virNetworkObjNew(void) +virNetworkObjNew(const char *uuidstr, + const char *name) { virNetworkObjPtr obj; if (virNetworkObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virNetworkObjClass))) + if (!(obj = virObjectLookupKeysNew(virNetworkObjClass, uuidstr, name))) return NULL; if (!(obj->classIdMap = virBitmapNew(INIT_CLASS_ID_BITMAP_SIZE))) @@ -158,7 +149,7 @@ virNetworkObjGetNewDef(virNetworkObjPtr obj) bool virNetworkObjIsActive(virNetworkObjPtr obj) { - return obj->active; + return virObjectLookupKeysIsActive(obj); } @@ -166,7 +157,7 @@ void virNetworkObjSetActive(virNetworkObjPtr obj, bool active) { - obj->active = active; + virObjectLookupKeysSetActive(obj, active); } @@ -332,36 +323,16 @@ virNetworkObjMacMgrDel(virNetworkObjPtr obj, virNetworkObjListPtr virNetworkObjListNew(void) { - virNetworkObjListPtr nets; - - if (virNetworkObjInitialize() < 0) - return NULL; - - if (!(nets = virObjectLockableNew(virNetworkObjListClass))) - return NULL; - - if (!(nets->objs = virHashCreate(50, virObjectFreeHashData))) { - virObjectUnref(nets); - return NULL; - } - - return nets; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 50, true); } static virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, - const unsigned char *uuid) + const char *uuidstr) { - virNetworkObjPtr obj = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(uuid, uuidstr); - - obj = virHashLookup(nets->objs, uuidstr); - if (obj) - virObjectRef(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(nets, uuidstr); + return (virNetworkObjPtr)obj; } @@ -379,30 +350,11 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { - virNetworkObjPtr obj; - - virObjectLock(nets); - obj = virNetworkObjFindByUUIDLocked(nets, uuid); - virObjectUnlock(nets); - if (obj) - virObjectLock(obj); - return obj; -} - - -static int -virNetworkObjSearchName(const void *payload, - const void *name ATTRIBUTE_UNUSED, - const void *data) -{ - virNetworkObjPtr obj = (virNetworkObjPtr) payload; - int want = 0; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(uuid, uuidstr); - virObjectLock(obj); - if (STREQ(obj->def->name, (const char *)data)) - want = 1; - virObjectUnlock(obj); - return want; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(nets, uuidstr); + return (virNetworkObjPtr)obj; } @@ -410,12 +362,8 @@ static virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj = NULL; - - obj = virHashSearch(nets->objs, virNetworkObjSearchName, name, NULL); - if (obj) - virObjectRef(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(nets, name); + return (virNetworkObjPtr)obj; } @@ -433,14 +381,8 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj; - - virObjectLock(nets); - obj = virNetworkObjFindByNameLocked(nets, name); - virObjectUnlock(nets); - if (obj) - virObjectLock(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(nets, name); + return (virNetworkObjPtr)obj; } @@ -470,15 +412,6 @@ virNetworkObjDispose(void *opaque) } -static void -virNetworkObjListDispose(void *opaque) -{ - virNetworkObjListPtr nets = opaque; - - virHashFree(nets->objs); -} - - /* * virNetworkObjUpdateAssignDef: * @network: the network object to update @@ -560,12 +493,12 @@ virNetworkObjAssignDefLocked(virNetworkObjListPtr nets, virNetworkObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(def->uuid, uuidstr); /* See if a network with matching UUID already exists */ - if ((obj = virNetworkObjFindByUUIDLocked(nets, def->uuid))) { + if ((obj = virNetworkObjFindByUUIDLocked(nets, uuidstr))) { virObjectLock(obj); /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(obj->def->name, def->name)) { - virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' is already defined with uuid %s"), obj->def->name, uuidstr); @@ -588,20 +521,17 @@ virNetworkObjAssignDefLocked(virNetworkObjListPtr nets, /* UUID does not match, but if a name matches, refuse it */ if ((obj = virNetworkObjFindByNameLocked(nets, def->name))) { virObjectLock(obj); - virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' already exists with uuid %s"), def->name, uuidstr); goto cleanup; } - if (!(obj = virNetworkObjNew())) - goto cleanup; + if (!(obj = virNetworkObjNew(uuidstr, def->name))) + goto cleanup; - virUUIDFormat(def->uuid, uuidstr); - if (virHashAddEntry(nets->objs, uuidstr, obj) < 0) + if (virObjectLookupHashAdd(nets, (virObjectLookupKeysPtr)obj) < 0) goto cleanup; - virObjectRef(obj); obj->def = def; obj->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE); @@ -638,9 +568,9 @@ virNetworkObjAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockWrite(nets); obj = virNetworkObjAssignDefLocked(nets, def, flags); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return obj; } @@ -784,16 +714,7 @@ void virNetworkObjRemoveInactive(virNetworkObjListPtr nets, virNetworkObjPtr obj) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(obj->def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); - virObjectLock(nets); - virObjectLock(obj); - virHashRemoveEntry(nets->objs, uuidstr); - virObjectUnlock(nets); - virObjectUnref(obj); + virObjectLookupHashRemove(nets, (virObjectLookupKeysPtr)obj); } @@ -968,7 +889,8 @@ virNetworkLoadState(virNetworkObjListPtr nets, obj->floor_sum = floor_sum_val; obj->taint = taint; - obj->active = true; /* network with a state file is by definition active */ + /* network with a state file is by definition active */ + virNetworkObjSetActive(obj, true); cleanup: VIR_FREE(configFile); @@ -1177,14 +1099,15 @@ virNetworkObjBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { + bool ret; virNetworkObjPtr obj; struct virNetworkObjBridgeInUseHelperData data = {bridge, skipname}; - virObjectLock(nets); - obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data, NULL); - virObjectUnlock(nets); + obj = (virNetworkObjPtr)virObjectLookupHashSearch(nets, virNetworkObjBridgeInUseHelper, &data); - return obj != NULL; + ret = obj != NULL; + virNetworkObjEndAPI(&obj); + return ret; } @@ -1309,21 +1232,14 @@ virNetworkMatch(virNetworkObjPtr obj, #undef MATCH -struct virNetworkObjListData { - virConnectPtr conn; - virNetworkPtr *nets; - virNetworkObjListFilter filter; - unsigned int flags; - int nnets; - bool error; -}; - static int -virNetworkObjListPopulate(void *payload, +virNetworkObjListExportCb(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + virNetworkPtr *nets = (virNetworkPtr *)data->elems; + virNetworkObjListFilter filter = data->filter; virNetworkObjPtr obj = payload; virNetworkPtr net = NULL; @@ -1332,15 +1248,14 @@ virNetworkObjListPopulate(void *payload, virObjectLock(obj); - if (data->filter && - !data->filter(data->conn, obj->def)) + if (filter && !filter(data->conn, obj->def)) goto cleanup; if (!virNetworkMatch(obj, data->flags)) goto cleanup; - if (!data->nets) { - data->nnets++; + if (!nets) { + data->nElems++; goto cleanup; } @@ -1349,7 +1264,7 @@ virNetworkObjListPopulate(void *payload, goto cleanup; } - data->nets[data->nnets++] = net; + nets[data->nElems++] = net; cleanup: virObjectUnlock(obj); @@ -1364,34 +1279,19 @@ virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags) { - int ret = -1; - struct virNetworkObjListData data = { - .conn = conn, .nets = NULL, .filter = filter, .flags = flags, - .nnets = 0, .error = false }; - - virObjectLock(netobjs); - if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) - goto cleanup; - - virHashForEach(netobjs->objs, virNetworkObjListPopulate, &data); + int ret; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - if (data.error) - goto cleanup; + if (nets) + data.maxElems = -1; - if (data.nets) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(data.nets, data.nnets + 1)); - *nets = data.nets; - data.nets = NULL; - } + ret = virObjectLookupHashForEach(netobjs, virNetworkObjListExportCb, &data); - ret = data.nnets; - cleanup: - virObjectUnlock(netobjs); - while (data.nets && data.nnets) - virObjectUnref(data.nets[--data.nnets]); + if (nets) + *nets = (virNetworkPtr *)data.elems; - VIR_FREE(data.nets); return ret; } @@ -1407,10 +1307,11 @@ virNetworkObjListForEachHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListForEachHelperData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + struct virNetworkObjListForEachHelperData *helperData = data->opaque; - if (data->callback(payload, data->opaque) < 0) - data->ret = -1; + if (helperData->callback(payload, helperData->opaque) < 0) + helperData->ret = -1; return 0; } @@ -1433,54 +1334,45 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, virNetworkObjListIterator callback, void *opaque) { - struct virNetworkObjListForEachHelperData data = { + struct virNetworkObjListForEachHelperData helperData = { .callback = callback, .opaque = opaque, .ret = 0}; - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); - virObjectUnlock(nets); - return data.ret; -} + virObjectLookupHashForEachData data = { + .opaque = &helperData, .error = false, .maxElems = 0 }; + return virObjectLookupHashForEach(nets, virNetworkObjListForEachHelper, + &data); +} -struct virNetworkObjListGetHelperData { - virConnectPtr conn; - virNetworkObjListFilter filter; - char **names; - int nnames; - int maxnames; - bool active; - bool error; -}; static int virNetworkObjListGetHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListGetHelperData *data = opaque; virNetworkObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + virNetworkObjListFilter filter = data->filter; + char **names = (char **)data->elems; if (data->error) return 0; - if (data->maxnames >= 0 && - data->nnames == data->maxnames) + if (data->maxElems >= 0 && + data->nElems == data->maxElems) return 0; virObjectLock(obj); - if (data->filter && - !data->filter(data->conn, obj->def)) + if (filter && !filter(data->conn, obj->def)) goto cleanup; - if ((data->active && virNetworkObjIsActive(obj)) || - (!data->active && !virNetworkObjIsActive(obj))) { - if (data->names && - VIR_STRDUP(data->names[data->nnames], obj->def->name) < 0) { + if ((data->wantActive && virNetworkObjIsActive(obj)) || + (!data->wantActive && !virNetworkObjIsActive(obj))) { + if (names && VIR_STRDUP(names[data->nElems], obj->def->name) < 0) { data->error = true; goto cleanup; } - data->nnames++; + data->nElems++; } cleanup: @@ -1497,26 +1389,11 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - int ret = -1; - - struct virNetworkObjListGetHelperData data = { - .conn = conn, .filter = filter, .names = names, .nnames = 0, - .maxnames = maxnames, .active = active, .error = false}; - - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); - - if (data.error) - goto cleanup; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .wantActive = active, .error = false, + .nElems = 0, .elems = (void **)names, .maxElems = maxnames }; - ret = data.nnames; - cleanup: - if (ret < 0) { - while (data.nnames) - VIR_FREE(data.names[--data.nnames]); - } - return ret; + return virObjectLookupHashForEach(nets, virNetworkObjListGetHelper, &data); } @@ -1526,15 +1403,11 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - struct virNetworkObjListGetHelperData data = { - .conn = conn, .filter = filter, .names = NULL, .nnames = 0, - .maxnames = -1, .active = active, .error = false}; - - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .wantActive = active, .error = false, + .nElems = 0, .elems = NULL, .maxElems = -2 }; - return data.nnames; + return virObjectLookupHashForEach(nets, virNetworkObjListGetHelper, &data); } @@ -1572,7 +1445,5 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, { struct virNetworkObjListPruneHelperData data = {flags}; - virObjectLock(nets); - virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); - virObjectUnlock(nets); + virObjectLookupHashPrune(nets, virNetworkObjListPruneHelper, &data); } diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index 627277b..050a184 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -28,7 +28,8 @@ typedef struct _virNetworkObj virNetworkObj; typedef virNetworkObj *virNetworkObjPtr; virNetworkObjPtr -virNetworkObjNew(void); +virNetworkObjNew(const char *uuidstr, + const char *name); virNetworkDefPtr virNetworkObjGetDef(virNetworkObjPtr obj); diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 4251a22..1a27aab 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -28,11 +28,13 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!(def = virNetworkDefParseFile(inxml))) goto fail; - if (!(obj = virNetworkObjNew())) + virUUIDFormat(def->uuid, uuidstr); + if (!(obj = virNetworkObjNew(uuidstr, def->name))) goto fail; virNetworkObjSetDef(obj, def); -- 2.9.4