[libvirt] [PATCH v5 00/15] virObject adjustments for common object
v4: https://www.redhat.com/archives/libvir-list/2017-August/msg00537.html Changes since v4... I've removed the virObjectLookupKeys completely relying on LookupHash completely. The adjusted LookupHash object uses named "objsUUID" and "objsName" as well as API's to specifically Search or ForEach by Name or UUID. The New, Add, and Remove API's were adjusted in order to handle creating things properly. Even if that adjustment isn't considered what's desired, at least look at patches 1, 10, and 14 separately. John Ferlan (15): util: Use VIR_ERROR instead of VIR_WARN util: Introduce virObjectLookupHash util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind[Locked] util: Introduce virObjectLookupHashForEach{UUID|Name} util: Introduce virObjectLookupHashSearch{UUID|Name}[Locked] nodedev: Use virObjectLookupHash secret: Use virObjectLookupHash util: Introduce virObjectLookupHashClone Revert "interface: Consume @def in virInterfaceObjNew" interface: Use virObjectLookupHash test: Clean up test driver Interface interactions util: Introduce virObjectLookupHashPrune network: Fix virNetworkObjBridgeInUse return type network: Use virObjectLookupHash src/conf/virinterfaceobj.c | 289 ++++++++++--------- src/conf/virnetworkobj.c | 280 ++++++------------ src/conf/virnetworkobj.h | 2 +- src/conf/virnodedeviceobj.c | 273 +++++------------- src/conf/virsecretobj.c | 251 +++++----------- src/libvirt_private.syms | 14 + src/test/test_driver.c | 55 +--- src/util/virobject.c | 675 +++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 123 ++++++++ 9 files changed, 1189 insertions(+), 773 deletions(-) -- 2.9.5
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@redhat.com> --- 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.5
Define an object meant to combine/manage the aspects of the various _vir*ObjList structs into one common purpose list management system that will store and manage _vir*Obj virObjectLockable objects rather than having multiple structs and API's to do that. The object will use the virObjectRWLockable class as the base and may have one or two hash tables that will be keyed by UUID or Name based on the needs of the consumer stored in: objsUUID -> Hash table for storing objects for lookup by UUID objsName -> Hash table for storing objects for lookup by Name The virObjectLookupHashNew will require the consumer to provide which hash tables are to be used via virObjectLookupHashNewFlags. 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@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 34 ++++++++++++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2149b11..33dbd92 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2318,6 +2318,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupHash; virClassForObjectRWLockable; virClassIsDerivedFrom; virClassName; @@ -2329,6 +2330,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashNew; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 38db692..edd8097 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -68,9 +68,11 @@ struct _virClass { static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectRWLockableClass; +static virClassPtr virObjectLookupHashClass; static void virObjectLockableDispose(void *anyobj); static void virObjectRWLockableDispose(void *anyobj); +static void virObjectLookupHashDispose(void *anyobj); static int virObjectOnceInit(void) @@ -93,6 +95,12 @@ virObjectOnceInit(void) virObjectRWLockableDispose))) return -1; + if (!(virObjectLookupHashClass = virClassNew(virObjectRWLockableClass, + "virObjectLookupHash", + sizeof(virObjectLookupHash), + virObjectLookupHashDispose))) + return -1; + return 0; } @@ -145,6 +153,21 @@ virClassForObjectRWLockable(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 @@ -328,6 +351,73 @@ virObjectRWLockableDispose(void *anyobj) /** + * virObjectLookupHashNew: + * @klass: the klass to check + * @tableElemsStart: initial size of each hash table + * @flags: virObjectLookupHashNewFlags to indicate which tables to create + * + * Create a new poolable hash table object for storing either 1 or 2 hash + * tables capable of storing virObjectLockable objects by UUID or Name. 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, + virObjectLookupHashNewFlags flags) +{ + virObjectLookupHashPtr obj; + + if (!flags || !(flags & (VIR_OBJECT_LOOKUP_HASH_UUID | + VIR_OBJECT_LOOKUP_HASH_NAME))) { + virReportError(VIR_ERR_INVALID_ARG, + _("flags=%x must be non zero or properly set"), flags); + return NULL; + } + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupHash())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupHash"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectRWLockableNew(klass))) + return NULL; + + if (flags & VIR_OBJECT_LOOKUP_HASH_UUID) { + if (!(obj->objsUUID = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + } + + if (flags & VIR_OBJECT_LOOKUP_HASH_NAME) { + if (!(obj->objsName = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + } + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupHashDispose(void *anyobj) +{ + virObjectLookupHashPtr obj = anyobj; + + virHashFree(obj->objsUUID); + virHashFree(obj->objsName); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -404,7 +494,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 ac6cf22..75efa90 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; @@ -37,6 +38,9 @@ typedef virObjectLockable *virObjectLockablePtr; typedef struct _virObjectRWLockable virObjectRWLockable; typedef virObjectRWLockable *virObjectRWLockablePtr; +typedef struct _virObjectLookupHash virObjectLookupHash; +typedef virObjectLookupHash *virObjectLookupHashPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -71,6 +75,24 @@ virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectRWLockable(void); +struct _virObjectLookupHash { + virObjectRWLockable parent; + + /* key1 string -> object mapping for O(1), + * lockless lookup-by-uuid */ + virHashTable *objsUUID; + + /* key2 string -> object mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; +}; + + +virClassPtr virClassForObject(void); +virClassPtr virClassForObjectLockable(void); +virClassPtr virClassForObjectRWLockable(void); +virClassPtr virClassForObjectLookupHash(void); + # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) # endif @@ -120,6 +142,18 @@ void * virObjectRWLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +typedef enum { + VIR_OBJECT_LOOKUP_HASH_UUID = (1 << 0), + VIR_OBJECT_LOOKUP_HASH_NAME = (1 << 1), +} virObjectLookupHashNewFlags; + +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart, + virObjectLookupHashNewFlags flags) + + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.5
Add a pair of API's to add/remove an object (virObjectLockable) 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@redhat.com> --- src/libvirt_private.syms | 2 + src/util/virobject.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 14 ++++++ 3 files changed, 142 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33dbd92..e98646c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2330,7 +2330,9 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashAdd; virObjectLookupHashNew; +virObjectLookupHashRemove; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index edd8097..76bf1bf 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -731,3 +731,129 @@ virObjectListFreeCount(void *list, VIR_FREE(list); } + + +static virObjectLookupHashPtr +virObjectGetLookupHashObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupHashClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLookupHashClass); + + return NULL; +} + + +static bool +virObjectLookupHashValidAddRemoveArgs(virObjectLookupHashPtr hashObj, + virObjectLockablePtr obj, + const char *uuidstr, + const char *name) +{ + if (!hashObj || !obj) + return false; + + if (uuidstr && !hashObj->objsUUID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsUUID for hashObj=%p, but uuidstr=%s provided"), + hashObj, uuidstr); + return false; + } + + if (name && !hashObj->objsName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsName for hashObj=%p, but name=%s provided"), + hashObj, name); + return false; + } + + return true; + +} + + +/** + * virObjectLookupHashAdd: + * @anyobj: LookupHash object + * @addObj: The (virObjectLockable) object to add to the hash table(s) + * @uuidstr: uuid formatted into a char string to add to UUID table + * @name: name to add to Name table + * + * Insert @obj into the hash tables found in @anyobj. Assumes that the + * caller has determined that @uuidstr and @name do not already exist + * in their respective hash table to be inserted. + * + * Returns 0 on success, -1 on failure. + */ +int +virObjectLookupHashAdd(void *anyobj, + void *addObj, + const char *uuidstr, + const char *name) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLockablePtr obj = virObjectGetLockableObj(addObj); + + if (!virObjectLookupHashValidAddRemoveArgs(hashObj, obj, uuidstr, name)) + return -1; + + if (hashObj->objsUUID) { + if (virHashAddEntry(hashObj->objsUUID, uuidstr, obj) < 0) + return -1; + virObjectRef(obj); + } + + if (hashObj->objsName) { + if (virHashAddEntry(hashObj->objsName, name, obj) < 0) { + if (hashObj->objsUUID) + virHashRemoveEntry(hashObj->objsUUID, uuidstr); + return -1; + } + virObjectRef(obj); + } + + return 0; +} + + +/** + * virObjectLookupHashRemove: + * @anyobj: LookupHash object + * @delObj: The (virObjectLockable) object to remove from the hash table(s) + * @uuidstr: uuid formatted into a char string to add to UUID table + * @name: name to add to Name table + * + * 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. + * + * NB: Caller must first check if @obj is NULL before calling. + * + * Even though this is a void, report the error for a bad @anyobj. + */ +void +virObjectLookupHashRemove(void *anyobj, + void *delObj, + const char *uuidstr, + const char *name) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLockablePtr obj = virObjectGetLockableObj(delObj); + + if (!virObjectLookupHashValidAddRemoveArgs(hashObj, obj, uuidstr, name)) + return; + + virObjectRef(obj); + virObjectUnlock(obj); + virObjectRWLockWrite(hashObj); + virObjectLock(obj); + if (uuidstr) + virHashRemoveEntry(hashObj->objsUUID, uuidstr); + if (name) + virHashRemoveEntry(hashObj->objsName, name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 75efa90..d149f30 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -181,4 +181,18 @@ void virObjectListFreeCount(void *list, size_t count); +int +virObjectLookupHashAdd(void *anyobj, + void *addObj, + const char *uuidstr, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void +virObjectLookupHashRemove(void *anyobj, + void *delObj, + const char *uuidstr, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_OBJECT_H */ -- 2.9.5
These API's will use the virHashLookup in order to find the object in the lookup hash object by the specified @key. If two hash tables exist in the hash table object, then both will be searched for the key. Both API's will call an virObjectLookupHashFindInternal in order in handle the fetch. The virObjectLookupHashFindLocked is the primary workhorse and should only be called externally if the caller has taken the proper RW LookupHash lock. This is necessary in some paths, such as during Add/Assign processing where getting the RW Write lock early is required to ensure no other thread attempts to create/add the same object Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 10 +++++++ 3 files changed, 85 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e98646c..8ad7223 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2331,6 +2331,8 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashFind; +virObjectLookupHashFindLocked; virObjectLookupHashNew; virObjectLookupHashRemove; virObjectNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 76bf1bf..63205d3 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -857,3 +857,76 @@ virObjectLookupHashRemove(void *anyobj, virObjectUnref(obj); virObjectRWUnlock(hashObj); } + + +static void * +virObjectLookupHashFindInternal(virObjectLookupHashPtr hashObj, + const char *key) +{ + virObjectLockablePtr obj; + + if (hashObj->objsUUID) { + if ((obj = virHashLookup(hashObj->objsUUID, key))) + return virObjectRef(obj); + } + + if (hashObj->objsName) { + obj = virHashLookup(hashObj->objsName, key); + return virObjectRef(obj); + } + + return NULL; +} + + +/** + * virObjectLookupHashFindLocked: + * @anyobj: LookupHash object + * @key: Key to use for lookup + * + * Search through the hash tables looking for the @key. The @key may be + * either UUID or Name - both tables if they exist will be searched. + * + * NB: Assumes that the LookupHash has already been locked + * + * Returns a pointer to the entry with refcnt incremented or NULL on failure + */ +void * +virObjectLookupHashFindLocked(void *anyobj, + const char *key) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return NULL; + + return virObjectLookupHashFindInternal(anyobj, key); + +} + + +/** + * virObjectLookupHashFind: + * @anyobj: LookupHash object + * @key: Key to use for lookup + * + * Call virObjectLookupHashFindLocked after locking the LookupHash + * + * Returns a pointer to the entry with refcnt incremented or NULL on failure + */ +void * +virObjectLookupHashFind(void *anyobj, + const char *key) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + void *obj; + + if (!hashObj) + return NULL; + + virObjectRWLockRead(hashObj); + obj = virObjectLookupHashFindInternal(hashObj, key); + virObjectRWUnlock(hashObj); + + return obj; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index d149f30..e5596e6 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -195,4 +195,14 @@ virObjectLookupHashRemove(void *anyobj, const char *name) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void * +virObjectLookupHashFindLocked(void *anyobj, + const char *key) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void * +virObjectLookupHashFind(void *anyobj, + const char *key) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_OBJECT_H */ -- 2.9.5
Introduce an API to use the virHashForEach API to go through each element of the LookupHash calling a callback for each 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{UUID|Name} => Callback function can be combined since generally all that the NumOf cares about is counts, but the Get function also adds some {UUID|Name} to a returned list of @uuids/@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@redhat.com> --- src/libvirt_private.syms | 2 + src/util/virobject.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 27 +++++++++++ 3 files changed, 147 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8ad7223..4d0883c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2333,6 +2333,8 @@ virObjectLockableNew; virObjectLookupHashAdd; virObjectLookupHashFind; virObjectLookupHashFindLocked; +virObjectLookupHashForEachName; +virObjectLookupHashForEachUUID; virObjectLookupHashNew; virObjectLookupHashRemove; virObjectNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 63205d3..dfd0bec 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -930,3 +930,121 @@ virObjectLookupHashFind(void *anyobj, return obj; } + + +static int +virObjectLookupHashForEachInternal(virHashTablePtr objsTable, + virHashIterator iter, + virObjectLookupHashForEachDataPtr data) +{ + if (data->maxElems == -1) { + if (VIR_ALLOC_N(data->elems, virHashSize(objsTable) + 1) < 0) + return -1; + } + + virHashForEach(objsTable, iter, data); + + 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; +} + + +/** + * virObjectLookupHashForEach{UUID|Name} + * @anyobj: LookupHash object + * @iter: callback function to handle the object specific checks + * @opaque: callback data + * + * For each element of the UUID or Name 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, but two API's are required since the caller + * can use either. + * + * 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 +virObjectLookupHashForEachUUID(void *anyobj, + virHashIterator iter, + virObjectLookupHashForEachDataPtr data) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + int ret; + + if (!hashObj) + return -1; + + if (!hashObj->objsUUID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsUUID for hashObj=%p"), hashObj); + return -1; + } + + virObjectRWLockRead(hashObj); + ret = virObjectLookupHashForEachInternal(hashObj->objsUUID, iter, data); + virObjectRWUnlock(hashObj); + + return ret; +} + + +int +virObjectLookupHashForEachName(void *anyobj, + virHashIterator iter, + virObjectLookupHashForEachDataPtr data) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + int ret; + + if (!hashObj) + return -1; + + if (!hashObj->objsName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsName for hashObj=%p"), hashObj); + return -1; + } + + virObjectRWLockRead(hashObj); + ret = virObjectLookupHashForEachInternal(hashObj->objsName, iter, data); + virObjectRWUnlock(hashObj); + + return ret; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index e5596e6..a35cf3c 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -205,4 +205,31 @@ 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 +virObjectLookupHashForEachUUID(void *anyobj, + virHashIterator iter, + virObjectLookupHashForEachDataPtr data) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int +virObjectLookupHashForEachName(void *anyobj, + virHashIterator iter, + virObjectLookupHashForEachDataPtr data) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_OBJECT_H */ -- 2.9.5
Add common object API wrappers to use the virHashSearch API to search the LookupHash for specific data defined in the @opaque parameter. Once data is found, the search would end and the object that is represented is returned locked with it's reference 's reference count incremented. The virObjectLookupHashSearch{UUID|Name}Locked is the workhorse, but similar to the virObjectLookupFind* APIs may be required an Add or AssignDef caller has the Write lock already to ensure no other thread will grab the lock and add the same or competing object. It is up to the caller to unlock and lower the refcnt once done with the object and of course handle a NULL return indicating no object found. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 4 ++ src/util/virobject.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 24 +++++++++ 3 files changed, 159 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4d0883c..b32004e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2337,6 +2337,10 @@ virObjectLookupHashForEachName; virObjectLookupHashForEachUUID; virObjectLookupHashNew; virObjectLookupHashRemove; +virObjectLookupHashSearchName; +virObjectLookupHashSearchNameLocked; +virObjectLookupHashSearchUUID; +virObjectLookupHashSearchUUIDLocked; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index dfd0bec..b20e938 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1048,3 +1048,134 @@ virObjectLookupHashForEachName(void *anyobj, return ret; } + + +static void * +virObjectLookupHashSearchInternal(virHashTablePtr objsTable, + virHashSearcher iter, + void *opaque) +{ + virObjectLockablePtr obj; + + obj = virHashSearch(objsTable, iter, opaque, NULL); + virObjectRef(obj); + + if (obj) + virObjectLock(obj); + + return obj; +} + + +/** + * virObjectLookupHashSearch{UUID|Name}Locked + * @anyobj: LookupHash object + * @iter: callback function to handle the object specific checks + * @opaque: callback data + * + * Search the hash table UUID or Name table calling the specified @iter + * routine with an object and @opaque data in order to determine whether + * the object is represented by the @opaque data. + * + * NB: Caller assumes the responsibility for locking LookupHash + * + * Returns locked/refcnt incremented object on success, NULL on failure + */ +void * +virObjectLookupHashSearchUUIDLocked(void *anyobj, + virHashSearcher iter, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return NULL; + + if (!hashObj->objsUUID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsUUID for hashObj=%p"), hashObj); + return NULL; + } + + return virObjectLookupHashSearchInternal(hashObj->objsUUID, iter, opaque); +} + + +void * +virObjectLookupHashSearchNameLocked(void *anyobj, + virHashSearcher iter, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return NULL; + + if (!hashObj->objsName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsName for hashObj=%p"), hashObj); + return NULL; + } + + return virObjectLookupHashSearchInternal(hashObj->objsName, iter, opaque); +} + + +/** + * virObjectLookupHashSearch{UUID|Name} + * @anyobj: LookupHash object + * @iter: callback function to handle the object specific checks + * @opaque: callback data + * + * Call virObjectLookupHashSearchLocked with a locked hash table + * + * Returns @obj from virObjectLookupHashSearchLocked + */ +void * +virObjectLookupHashSearchUUID(void *anyobj, + virHashSearcher iter, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLockablePtr obj; + + if (!hashObj) + return NULL; + + if (!hashObj->objsUUID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsUUID for hashObj=%p"), hashObj); + return NULL; + } + + virObjectRWLockRead(hashObj); + obj = virObjectLookupHashSearchInternal(hashObj->objsUUID, iter, opaque); + virObjectRWUnlock(hashObj); + + return obj; +} + + +void * +virObjectLookupHashSearchName(void *anyobj, + virHashSearcher iter, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLockablePtr obj; + + if (!hashObj) + return NULL; + + if (!hashObj->objsName) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no objsName for hashObj=%p"), hashObj); + return NULL; + } + + virObjectRWLockRead(hashObj); + obj = virObjectLookupHashSearchInternal(hashObj->objsName, iter, opaque); + virObjectRWUnlock(hashObj); + + return obj; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index a35cf3c..03a23e0 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -232,4 +232,28 @@ virObjectLookupHashForEachName(void *anyobj, virObjectLookupHashForEachDataPtr data) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void * +virObjectLookupHashSearchUUIDLocked(void *anyobj, + virHashSearcher iter, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void * +virObjectLookupHashSearchNameLocked(void *anyobj, + virHashSearcher iter, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void * +virObjectLookupHashSearchUUID(void *anyobj, + virHashSearcher iter, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void * +virObjectLookupHashSearchName(void *anyobj, + virHashSearcher iter, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_OBJECT_H */ -- 2.9.5
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. 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@redhat.com> --- src/conf/virnodedeviceobj.c | 273 ++++++++++++-------------------------------- 1 file changed, 75 insertions(+), 198 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b0dcee1..b97518b 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -40,19 +40,12 @@ struct _virNodeDeviceObj { }; 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) @@ -63,12 +56,6 @@ virNodeDeviceObjOnceInit(void) virNodeDeviceObjDispose))) return -1; - if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), - "virNodeDeviceObjList", - sizeof(virNodeDeviceObjList), - virNodeDeviceObjListDispose))) - return -1; - return 0; } @@ -229,17 +216,7 @@ 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; + return virObjectLookupHashSearchName(devs, callback, (void *)data); } @@ -274,7 +251,7 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, const char *name) { - return virObjectRef(virHashLookup(devs->objs, name)); + return virObjectLookupHashFindLocked(devs, name); } @@ -282,15 +259,7 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - virNodeDeviceObjPtr obj; - - virObjectLock(devs); - obj = virNodeDeviceObjListFindByNameLocked(devs, name); - virObjectUnlock(devs); - if (obj) - virObjectLock(obj); - - return obj; + return virObjectLookupHashFind(devs, name); } @@ -445,32 +414,11 @@ 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, + VIR_OBJECT_LOOKUP_HASH_NAME); } @@ -486,29 +434,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())) goto cleanup; - if (virHashAddEntry(devs->objs, def->name, obj) < 0) { - virNodeDeviceObjEndAPI(&obj); + if (virObjectLookupHashAdd(devs, obj, NULL, def->name) < 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 +466,11 @@ 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); + /* @obj is already locked on entry */ + virObjectLookupHashRemove(devs, obj, NULL, obj->def->name); } @@ -730,29 +671,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 +709,12 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, const char *cap, virNodeDeviceObjListFilter filter) { - struct virNodeDeviceCountData data = { - .conn = conn, .filter = filter, .matchstr = cap, .count = 0 }; - - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); - virObjectUnlock(devs); + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = NULL, .maxElems = -2 }; - 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 virObjectLookupHashForEachName(devs, virNodeDeviceObjListGetHelper, + &data); } @@ -824,23 +726,12 @@ 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 }; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = (void **)names, .maxElems = maxnames }; - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); - virObjectUnlock(devs); - - if (data.error) - goto error; - - return data.nnames; - - error: - while (--data.nnames) - VIR_FREE(data.names[data.nnames]); - return -1; + return virObjectLookupHashForEachName(devs, virNodeDeviceObjListGetHelper, + &data); } @@ -876,15 +767,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 +774,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,20 +785,25 @@ 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); return 0; @@ -928,31 +817,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 = virObjectLookupHashForEachName(devs, virNodeDeviceObjListExportCallback, + &data); - return data.ndevices; + if (devices) + *devices = (virNodeDevicePtr *)data.elems; - cleanup: - virObjectListFree(data.devices); - return -1; + return ret; } -- 2.9.5
Use the virObjectLookupHash in _virSecretObjList. Convert the code to use the LookupHash object and APIs rather than local code and usage of the virHash* calls. Since the _virSecretObjList 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} API's returns a locked/reffed object so need to remove virObjectLock after FindBy*Locked calls. The only function requiring a 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 secret via UUID or UsageID at the same time. NB: We cannot make usageID a LookupHash key because it's possible that the usageType is NONE and thus usageID is NULL, thus leaving the only "unique" element the UUID. The NumOfSecretsCallback and GetUUIDsCallback can use the same callback function with the only difference being the filling in of the @uuids array from the passed @data structure if it exists. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 251 ++++++++++++------------------------------------ 1 file changed, 64 insertions(+), 187 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 4dca152..21a5ab7 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -47,16 +47,10 @@ struct _virSecretObj { }; static virClassPtr virSecretObjClass; -static virClassPtr virSecretObjListClass; static void virSecretObjDispose(void *obj); -static void virSecretObjListDispose(void *obj); struct _virSecretObjList { - virObjectLockable parent; - - /* uuid string -> virSecretObj mapping - * for O(1), lockless lookup-by-uuid */ - virHashTable *objs; + virObjectLookupHash parent; }; struct virSecretSearchData { @@ -74,12 +68,6 @@ virSecretObjOnceInit(void) virSecretObjDispose))) return -1; - if (!(virSecretObjListClass = virClassNew(virClassForObjectLockable(), - "virSecretObjList", - sizeof(virSecretObjList), - virSecretObjListDispose))) - return -1; - return 0; } @@ -118,20 +106,8 @@ virSecretObjEndAPI(virSecretObjPtr *obj) virSecretObjListPtr virSecretObjListNew(void) { - virSecretObjListPtr secrets; - - if (virSecretObjInitialize() < 0) - return NULL; - - if (!(secrets = virObjectLockableNew(virSecretObjListClass))) - return NULL; - - if (!(secrets->objs = virHashCreate(50, virObjectFreeHashData))) { - virObjectUnref(secrets); - return NULL; - } - - return secrets; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 50, + VIR_OBJECT_LOOKUP_HASH_UUID); } @@ -151,15 +127,6 @@ virSecretObjDispose(void *opaque) } -static void -virSecretObjListDispose(void *obj) -{ - virSecretObjListPtr secrets = obj; - - virHashFree(secrets->objs); -} - - /** * virSecretObjFindByUUIDLocked: * @secrets: list of secret objects @@ -173,7 +140,7 @@ static virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, const char *uuidstr) { - return virObjectRef(virHashLookup(secrets->objs, uuidstr)); + return virObjectLookupHashFindLocked(secrets, uuidstr); } @@ -191,14 +158,7 @@ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, const char *uuidstr) { - virSecretObjPtr obj; - - virObjectLock(secrets); - obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); - virObjectUnlock(secrets); - if (obj) - virObjectLock(obj); - return obj; + return virObjectLookupHashFind(secrets, uuidstr); } @@ -243,14 +203,11 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr obj = NULL; struct virSecretSearchData data = { .usageType = usageType, .usageID = usageID }; - obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data, NULL); - if (obj) - virObjectRef(obj); - return obj; + return virObjectLookupHashSearchUUIDLocked(secrets, virSecretObjSearchName, + &data); } @@ -263,6 +220,12 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, * This function locks @secrets and finds the secret object which * corresponds to @usageID of @usageType. * + * NB: The usageID cannot be used as a hash table key because + * virSecretDefParseUsage will not fill in the def->usage_id + * if the def->usage_type is VIR_SECRET_USAGE_TYPE_NONE, thus + * we cannot use def->usage_id as a key since both keys must + * be present in every object in order to be valid. + * * Returns: locked and ref'd secret object. */ virSecretObjPtr @@ -270,21 +233,17 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr obj; + struct virSecretSearchData data = { .usageType = usageType, + .usageID = usageID }; - virObjectLock(secrets); - obj = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); - virObjectUnlock(secrets); - if (obj) - virObjectLock(obj); - return obj; + return virObjectLookupHashSearchUUID(secrets, virSecretObjSearchName, &data); } /* * virSecretObjListRemove: * @secrets: list of secret objects - * @secret: a secret object + * @obj: a locked secret object * * Remove the object from the hash table. The caller must hold the lock * on the driver owning @secrets and must have also locked @secret to @@ -295,22 +254,12 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virSecretObjPtr obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virSecretDefPtr def; if (!obj) return; - def = obj->def; - virUUIDFormat(def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); - - virObjectLock(secrets); - virObjectLock(obj); - virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(obj); - virObjectUnref(obj); - virObjectUnlock(secrets); + virUUIDFormat(obj->def->uuid, uuidstr); + virObjectLookupHashRemove(secrets, obj, uuidstr, NULL); } @@ -336,7 +285,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virObjectLock(secrets); + virObjectRWLockWrite(secrets); if (oldDef) *oldDef = NULL; @@ -345,7 +294,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, /* Is there a secret already matching this UUID */ if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { - virObjectLock(obj); objdef = obj->def; if (STRNEQ_NULLABLE(objdef->usage_id, newdef->usage_id)) { @@ -373,7 +321,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, if ((obj = virSecretObjListFindByUsageLocked(secrets, newdef->usage_type, newdef->usage_id))) { - virObjectLock(obj); objdef = obj->def; virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, @@ -393,11 +340,10 @@ virSecretObjListAdd(virSecretObjListPtr secrets, !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + if (virObjectLookupHashAdd(secrets, obj, uuidstr, NULL) < 0) goto cleanup; obj->def = newdef; - virObjectRef(obj); } ret = obj; @@ -405,72 +351,35 @@ virSecretObjListAdd(virSecretObjListPtr secrets, cleanup: virSecretObjEndAPI(&obj); - virObjectUnlock(secrets); + virObjectRWUnlock(secrets); return ret; } -struct virSecretCountData { - virConnectPtr conn; - virSecretObjListACLFilter filter; - int count; -}; - static int -virSecretObjListNumOfSecretsCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +virSecretObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { - struct virSecretCountData *data = opaque; - virSecretObjPtr obj = payload; - virSecretDefPtr def; - - virObjectLock(obj); - def = obj->def; - - if (data->filter && !data->filter(data->conn, def)) - goto cleanup; - - data->count++; - - cleanup: - virObjectUnlock(obj); - return 0; -} - - -struct virSecretListData { - virConnectPtr conn; - virSecretObjListACLFilter filter; - int nuuids; - char **uuids; - int maxuuids; - bool error; -}; - - -static int -virSecretObjListGetUUIDsCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) -{ - struct virSecretListData *data = opaque; virSecretObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + virSecretObjListACLFilter filter = data->filter; + char **uuids = (char **)data->elems; virSecretDefPtr def; if (data->error) return 0; - if (data->maxuuids >= 0 && data->nuuids == data->maxuuids) + if (data->maxElems >= 0 && data->nElems == data->maxElems) return 0; virObjectLock(obj); def = obj->def; - if (data->filter && !data->filter(data->conn, def)) + if (filter && !filter(data->conn, def)) goto cleanup; - if (data->uuids) { + if (uuids) { char *uuidstr; if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) { @@ -479,7 +388,9 @@ virSecretObjListGetUUIDsCallback(void *payload, } virUUIDFormat(def->uuid, uuidstr); - data->uuids[data->nuuids++] = uuidstr; + uuids[data->nElems++] = uuidstr; + } else { + data->nElems++; } cleanup: @@ -493,14 +404,12 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn) { - struct virSecretCountData data = { - .conn = conn, .filter = filter, .count = 0 }; - - virObjectLock(secrets); - virHashForEach(secrets->objs, virSecretObjListNumOfSecretsCallback, &data); - virObjectUnlock(secrets); + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = -2 }; - return data.count; + return virObjectLookupHashForEachUUID(secrets, virSecretObjListGetHelper, + &data); } @@ -532,22 +441,15 @@ virSecretObjMatchFlags(virSecretObjPtr obj, #undef MATCH -struct virSecretObjListData { - virConnectPtr conn; - virSecretPtr *secrets; - virSecretObjListACLFilter filter; - unsigned int flags; - int nsecrets; - bool error; -}; - static int virSecretObjListExportCallback(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virSecretObjListData *data = opaque; virSecretObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + virSecretPtr *secrets = (virSecretPtr *)data->elems; + virSecretObjListACLFilter filter = data->filter; virSecretDefPtr def; virSecretPtr secret = NULL; @@ -557,25 +459,24 @@ virSecretObjListExportCallback(void *payload, virObjectLock(obj); def = obj->def; - if (data->filter && !data->filter(data->conn, def)) + if (filter && !filter(data->conn, def)) goto cleanup; if (!virSecretObjMatchFlags(obj, data->flags)) goto cleanup; - if (!data->secrets) { - data->nsecrets++; + if (!secrets) { + data->nElems++; goto cleanup; } if (!(secret = virGetSecret(data->conn, def->uuid, - def->usage_type, - def->usage_id))) { + def->usage_type, def->usage_id))) { data->error = true; goto cleanup; } - data->secrets[data->nsecrets++] = secret; + secrets[data->nElems++] = secret; cleanup: virObjectUnlock(obj); @@ -590,35 +491,22 @@ virSecretObjListExport(virConnectPtr conn, virSecretObjListACLFilter filter, unsigned int flags) { - struct virSecretObjListData data = { - .conn = conn, .secrets = NULL, - .filter = filter, .flags = flags, - .nsecrets = 0, .error = false }; - - virObjectLock(secretobjs); - if (secrets && - VIR_ALLOC_N(data.secrets, virHashSize(secretobjs->objs) + 1) < 0) { - virObjectUnlock(secretobjs); - return -1; - } + int ret; - virHashForEach(secretobjs->objs, virSecretObjListExportCallback, &data); - virObjectUnlock(secretobjs); + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - if (data.error) - goto error; - - if (data.secrets) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(data.secrets, data.nsecrets + 1)); - *secrets = data.secrets; - } + if (secrets) + data.maxElems = -1; - return data.nsecrets; + ret = virObjectLookupHashForEachUUID(secretobjs, + virSecretObjListExportCallback, + &data); + if (secrets) + *secrets = (virSecretPtr *)data.elems; - error: - virObjectListFree(data.secrets); - return -1; + return ret; } @@ -629,23 +517,12 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn) { - struct virSecretListData data = { - .conn = conn, .filter = filter, .uuids = uuids, .nuuids = 0, - .maxuuids = maxuuids, .error = false }; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = (void **)uuids, .maxElems = maxuuids }; - virObjectLock(secrets); - virHashForEach(secrets->objs, virSecretObjListGetUUIDsCallback, &data); - virObjectUnlock(secrets); - - if (data.error) - goto error; - - return data.nuuids; - - error: - while (--data.nuuids) - VIR_FREE(data.uuids[data.nuuids]); - return -1; + return virObjectLookupHashForEachUUID(secrets, virSecretObjListGetHelper, + &data); } -- 2.9.5
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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 9 ++++++ 3 files changed, 93 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b32004e..ecbd84a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2331,6 +2331,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashClone; virObjectLookupHashFind; virObjectLookupHashFindLocked; virObjectLookupHashForEachName; diff --git a/src/util/virobject.c b/src/util/virobject.c index b20e938..7152b17 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1179,3 +1179,86 @@ virObjectLookupHashSearchName(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) +{ + virObjectLockablePtr 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 entry + * and call the driver specific @cb function with the element from the + * source hash table in order to clone into the destination hash table. + * This will be done for each hash table that exists. + * + * 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); + if (srcHashObj->objsUUID) + virHashForEach(srcHashObj->objsUUID, cloneCallback, &data); + if (srcHashObj->objsName) + virHashForEach(srcHashObj->objsName, cloneCallback, &data); + virObjectRWUnlock(srcHashObj); + + if (data.error) + return -1; + + return 0; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 03a23e0..66b44ca 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -256,4 +256,13 @@ virObjectLookupHashSearchName(void *anyobj, void *opaque) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +typedef int (*virObjectLookupHashCloneCallback)(void *dstHashTable, + 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.5
This reverts commit 92840eb3a7e47cdf761e52afccc41d2a35327fbd. More recent reviews/changes don't have the vir*ObjNew APIs consuming the @def, so remove from Interface as well. Changes needed to also deal with conflicts from commit id '46f5eca4'. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 106f232..e993b92 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -74,7 +74,7 @@ virInterfaceObjDispose(void *opaque) static virInterfaceObjPtr -virInterfaceObjNew(virInterfaceDefPtr def) +virInterfaceObjNew(void) { virInterfaceObjPtr obj; @@ -85,7 +85,6 @@ virInterfaceObjNew(virInterfaceDefPtr def) return NULL; virObjectLock(obj); - obj->def = def; return obj; } @@ -261,15 +260,15 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, return obj; } - if (!(obj = virInterfaceObjNew(def))) + if (!(obj = virInterfaceObjNew())) return NULL; if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, interfaces->count, obj) < 0) { - obj->def = NULL; virInterfaceObjEndAPI(&obj); return NULL; } + obj->def = def; return virObjectRef(obj); } -- 2.9.5
On Wed, Aug 23, 2017 at 05:22:06PM -0400, John Ferlan wrote:
This reverts commit 92840eb3a7e47cdf761e52afccc41d2a35327fbd.
More recent reviews/changes don't have the vir*ObjNew APIs consuming the @def, so remove from Interface as well. Changes needed to also deal with conflicts from commit id '46f5eca4'.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 LookupHash 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@redhat.com> --- src/conf/virinterfaceobj.c | 286 ++++++++++++++++++++++++--------------------- 1 file changed, 150 insertions(+), 136 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e993b92..a9b37d9 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -40,8 +40,7 @@ struct _virInterfaceObj { }; struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; + virObjectLookupHash parent; }; /* virInterfaceObj manipulation */ @@ -128,11 +127,41 @@ virInterfaceObjSetActive(virInterfaceObjPtr obj, virInterfaceObjListPtr virInterfaceObjListNew(void) { - virInterfaceObjListPtr interfaces; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 10, + VIR_OBJECT_LOOKUP_HASH_NAME); +} - 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 +171,21 @@ 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 virObjectLookupHashForEachName(interfaces, + virInterfaceObjListFindByMACStringCb, + &data); +} - error: - while (--matchct >= 0) - VIR_FREE(matches[matchct]); - return -1; +static virInterfaceObjPtr +virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces, + const char *name) +{ + return virObjectLookupHashFindLocked(interfaces, name); } @@ -176,73 +193,66 @@ 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; + return virObjectLookupHashFind(interfaces, name); } void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) { - size_t i; + virObjectUnref(interfaces); +} + + +static int +virInterfaceObjListCloneCb(void *dstHashTable, + void *sourceObject) +{ + virInterfaceObjListPtr dest = dstHashTable; + virInterfaceObjPtr srcObj = sourceObject; + int ret = -1; + char *xml = NULL; + virInterfaceObjPtr obj; + virInterfaceDefPtr backup = NULL; + + if (!(xml = virInterfaceDefFormat(srcObj->def))) + goto cleanup; + + if (!(backup = virInterfaceDefParseString(xml))) + goto cleanup; + + if (!(obj = virInterfaceObjListAssignDef(dest, backup))) + goto cleanup; - for (i = 0; i < interfaces->count; i++) - virObjectUnref(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - VIR_FREE(interfaces); + 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; + return destInterfaces; - if (!(backup = virInterfaceDefParseString(xml))) { - VIR_FREE(xml); - goto error; - } - - VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) - goto error; - virInterfaceObjEndAPI(&obj); - } - - return dest; - - error: - virInterfaceObjListFree(dest); + cleanup: + virObjectUnref(destInterfaces); return NULL; } @@ -252,24 +262,29 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { virInterfaceObjPtr obj; + virInterfaceObjPtr ret = NULL; - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + virObjectRWLockWrite(interfaces); + + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + } else { + if (!(obj = virInterfaceObjNew())) + goto cleanup; - return obj; + if (virObjectLookupHashAdd(interfaces, obj, NULL, def->name) < 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 +292,43 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i; + if (!obj) + return; - 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; + /* @obj is locked upon entry */ + virObjectLookupHashRemove(interfaces, obj, NULL, obj->def->name); +} + + +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 +336,13 @@ 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 virObjectLookupHashForEachName(interfaces, + virInterfaceObjListGetHelper, + &data); } @@ -319,30 +352,11 @@ 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 virObjectLookupHashForEachName(interfaces, + virInterfaceObjListGetHelper, + &data); } -- 2.9.5
Now that we have self locking hash table for Interface object lookups, there's no need to take the test driver lock in order to "lock" between the various API's, so remove the Lock/Unlock logic accordingly. Add a virInterfaceObjListFree during testDriverFree for the backupIfaces "just in case". Also use the VIR_STEAL_PTR rather than inline the code. Finally alter a couple of "if ((obj = function()) == NULL" to use the more standard "if (!(obj = function()))" syntax. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 55 ++++++++++++-------------------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index aa38f54..70977b8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -155,6 +155,7 @@ testDriverFree(testDriverPtr driver) virNodeDeviceObjListFree(driver->devs); virObjectUnref(driver->networks); virInterfaceObjListFree(driver->ifaces); + virInterfaceObjListFree(driver->backupIfaces); virStoragePoolObjListFree(&driver->pools); virObjectUnref(driver->eventState); virMutexUnlock(&driver->lock); @@ -3689,11 +3690,7 @@ testInterfaceObjFindByName(testDriverPtr privconn, { virInterfaceObjPtr obj; - testDriverLock(privconn); - obj = virInterfaceObjListFindByName(privconn->ifaces, name); - testDriverUnlock(privconn); - - if (!obj) + if (!(obj = virInterfaceObjListFindByName(privconn->ifaces, name))) virReportError(VIR_ERR_NO_INTERFACE, _("no interface with matching name '%s'"), name); @@ -3706,12 +3703,8 @@ static int testConnectNumOfInterfaces(virConnectPtr conn) { testDriverPtr privconn = conn->privateData; - int ninterfaces; - testDriverLock(privconn); - ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, true); - testDriverUnlock(privconn); - return ninterfaces; + return virInterfaceObjListNumOfInterfaces(privconn->ifaces, true); } @@ -3721,14 +3714,8 @@ testConnectListInterfaces(virConnectPtr conn, int maxnames) { testDriverPtr privconn = conn->privateData; - int nnames; - testDriverLock(privconn); - nnames = virInterfaceObjListGetNames(privconn->ifaces, true, - names, maxnames); - testDriverUnlock(privconn); - - return nnames; + return virInterfaceObjListGetNames(privconn->ifaces, true, names, maxnames); } @@ -3736,12 +3723,8 @@ static int testConnectNumOfDefinedInterfaces(virConnectPtr conn) { testDriverPtr privconn = conn->privateData; - int ninterfaces; - testDriverLock(privconn); - ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, false); - testDriverUnlock(privconn); - return ninterfaces; + return virInterfaceObjListNumOfInterfaces(privconn->ifaces, false); } @@ -3751,14 +3734,8 @@ testConnectListDefinedInterfaces(virConnectPtr conn, int maxnames) { testDriverPtr privconn = conn->privateData; - int nnames; - testDriverLock(privconn); - nnames = virInterfaceObjListGetNames(privconn->ifaces, false, - names, maxnames); - testDriverUnlock(privconn); - - return nnames; + return virInterfaceObjListGetNames(privconn->ifaces, false, names, maxnames); } @@ -3791,12 +3768,8 @@ testInterfaceLookupByMACString(virConnectPtr conn, char *ifacenames[] = { NULL, NULL }; virInterfacePtr ret = NULL; - testDriverLock(privconn); - ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, - ifacenames, 2); - testDriverUnlock(privconn); - - if (ifacect == 0) { + if ((ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac, + ifacenames, 2)) == 0) { virReportError(VIR_ERR_NO_INTERFACE, _("no interface with matching mac '%s'"), mac); goto cleanup; @@ -3880,6 +3853,7 @@ testInterfaceChangeCommit(virConnectPtr conn, } virInterfaceObjListFree(privconn->backupIfaces); + privconn->backupIfaces = NULL; privconn->transaction_running = false; ret = 0; @@ -3910,8 +3884,7 @@ testInterfaceChangeRollback(virConnectPtr conn, } virInterfaceObjListFree(privconn->ifaces); - privconn->ifaces = privconn->backupIfaces; - privconn->backupIfaces = NULL; + VIR_STEAL_PTR(privconn->ifaces, privconn->backupIfaces); privconn->transaction_running = false; @@ -3958,11 +3931,10 @@ testInterfaceDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); - testDriverLock(privconn); - if ((def = virInterfaceDefParseString(xmlStr)) == NULL) - goto cleanup; + if (!(def = virInterfaceDefParseString(xmlStr))) + return NULL; - if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL) + if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) goto cleanup; def = NULL; objdef = virInterfaceObjGetDef(obj); @@ -3972,7 +3944,6 @@ testInterfaceDefineXML(virConnectPtr conn, cleanup: virInterfaceDefFree(def); virInterfaceObjEndAPI(&obj); - testDriverUnlock(privconn); return ret; } -- 2.9.5
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 objsUUID - if something goes wrong in objsName, the best that can be done is to issue an error message and hope for the best. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 5 +++++ 3 files changed, 45 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ecbd84a..e4c465f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2337,6 +2337,7 @@ virObjectLookupHashFindLocked; virObjectLookupHashForEachName; virObjectLookupHashForEachUUID; virObjectLookupHashNew; +virObjectLookupHashPrune; virObjectLookupHashRemove; virObjectLookupHashSearchName; virObjectLookupHashSearchNameLocked; diff --git a/src/util/virobject.c b/src/util/virobject.c index 7152b17..c259ccc 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1262,3 +1262,42 @@ 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 + * the same number of elements, then issue an ERROR message, but continue + * on as there's not much we can do now to restore the data other than + * attempt to let someone know. + */ +void +virObjectLookupHashPrune(void *anyobj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + ssize_t cntUUIDs = 0; + ssize_t cntNames = 0; + + if (!hashObj) + return; + + virObjectRWLockWrite(hashObj); + if (hashObj->objsUUID) + cntUUIDs = virHashRemoveSet(hashObj->objsUUID, callback, opaque); + + if (hashObj->objsName) + cntNames = virHashRemoveSet(hashObj->objsName, callback, opaque); + + if (hashObj->objsUUID && hashObj->objsName && cntNames != cntUUIDs) + VIR_ERROR(_("removed %zd elems from objsUUID and %zd elems " + "from objsName"), cntUUIDs, cntNames); + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 66b44ca..a6038fc 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -264,5 +264,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.5
Rather than an int, it returns a bool - so let's define it that way Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnetworkobj.c | 2 +- src/conf/virnetworkobj.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 2bd69dc..20f846d 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1172,7 +1172,7 @@ virNetworkObjBridgeInUseHelper(const void *payload, } -int +bool virNetworkObjBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index 463be26..627277b 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -174,7 +174,7 @@ virNetworkObjDeleteConfig(const char *configDir, const char *autostartDir, virNetworkObjPtr net); -int +bool virNetworkObjBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname); -- 2.9.5
On Wed, Aug 23, 2017 at 05:22:10PM -0400, John Ferlan wrote:
Rather than an int, it returns a bool - so let's define it that way
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnetworkobj.c | 2 +- src/conf/virnetworkobj.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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@redhat.com> --- src/conf/virnetworkobj.c | 278 ++++++++++++++--------------------------------- 1 file changed, 79 insertions(+), 199 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 20f846d..a5c7d19 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -61,15 +61,11 @@ 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) @@ -80,11 +76,6 @@ virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), - "virNetworkObjList", - sizeof(virNetworkObjList), - virNetworkObjListDispose))) - return -1; return 0; } @@ -332,36 +323,17 @@ 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, + VIR_OBJECT_LOOKUP_HASH_UUID | + VIR_OBJECT_LOOKUP_HASH_NAME); } 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; + return virObjectLookupHashFindLocked(nets, uuidstr); } @@ -379,30 +351,10 @@ 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; + return virObjectLookupHashFind(nets, uuidstr); } @@ -410,12 +362,7 @@ static virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj = NULL; - - obj = virHashSearch(nets->objs, virNetworkObjSearchName, name, NULL); - if (obj) - virObjectRef(obj); - return obj; + return virObjectLookupHashFindLocked(nets, name); } @@ -433,14 +380,7 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj; - - virObjectLock(nets); - obj = virNetworkObjFindByNameLocked(nets, name); - virObjectUnlock(nets); - if (obj) - virObjectLock(obj); - return obj; + return virObjectLookupHashFind(nets, name); } @@ -470,15 +410,6 @@ virNetworkObjDispose(void *opaque) } -static void -virNetworkObjListDispose(void *opaque) -{ - virNetworkObjListPtr nets = opaque; - - virHashFree(nets->objs); -} - - /* * virNetworkObjUpdateAssignDef: * @network: the network object to update @@ -560,12 +491,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,7 +519,6 @@ 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); @@ -596,12 +526,10 @@ virNetworkObjAssignDefLocked(virNetworkObjListPtr nets, } if (!(obj = virNetworkObjNew())) - goto cleanup; + goto cleanup; - virUUIDFormat(def->uuid, uuidstr); - if (virHashAddEntry(nets->objs, uuidstr, obj) < 0) + if (virObjectLookupHashAdd(nets, obj, uuidstr, def->name) < 0) goto cleanup; - virObjectRef(obj); obj->def = def; obj->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE); @@ -638,9 +566,9 @@ virNetworkObjAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockWrite(nets); obj = virNetworkObjAssignDefLocked(nets, def, flags); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return obj; } @@ -786,14 +714,12 @@ virNetworkObjRemoveInactive(virNetworkObjListPtr nets, { char uuidstr[VIR_UUID_STRING_BUFLEN]; + if (!obj) + return; + + /* @obj is already locked on entry */ virUUIDFormat(obj->def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); - virObjectLock(nets); - virObjectLock(obj); - virHashRemoveEntry(nets->objs, uuidstr); - virObjectUnlock(nets); - virObjectUnref(obj); + virObjectLookupHashRemove(nets, obj, uuidstr, obj->def->name); } @@ -968,7 +894,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 +1104,16 @@ 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 = virObjectLookupHashSearchUUID(nets, virNetworkObjBridgeInUseHelper, + &data); - return obj != NULL; + ret = obj != NULL; + virNetworkObjEndAPI(&obj); + return ret; } @@ -1309,21 +1238,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 +1254,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 +1270,7 @@ virNetworkObjListPopulate(void *payload, goto cleanup; } - data->nets[data->nnets++] = net; + nets[data->nElems++] = net; cleanup: virObjectUnlock(obj); @@ -1364,34 +1285,20 @@ 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; + int ret; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - virHashForEach(netobjs->objs, virNetworkObjListPopulate, &data); + if (nets) + data.maxElems = -1; - if (data.error) - goto cleanup; - - 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 = virObjectLookupHashForEachUUID(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 +1314,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 +1341,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 virObjectLookupHashForEachUUID(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 +1396,12 @@ 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 virObjectLookupHashForEachUUID(nets, virNetworkObjListGetHelper, + &data); } @@ -1526,15 +1411,12 @@ 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 virObjectLookupHashForEachUUID(nets, virNetworkObjListGetHelper, + &data); } @@ -1572,7 +1454,5 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, { struct virNetworkObjListPruneHelperData data = {flags}; - virObjectLock(nets); - virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); - virObjectUnlock(nets); + virObjectLookupHashPrune(nets, virNetworkObjListPruneHelper, &data); } -- 2.9.5
participants (2)
-
John Ferlan -
Pavel Hrdina