[libvirt] [PATCH v4 00/17] virObject adjustments for common object
v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html Changes since v3 - honestly it's been too long to remember exactly what changes have taken place. This series provide the util/virobject changes and the implementation for nodedev, secret, interface, and network drivers/vir*obj's. The nwfilter is awaiting more upstream review and work and some more Storage Pool/Volume patches will be posted in conjunction with these. These changes also use the rwlocks Michal recently added to virobject as the means to provide locks for the LookupHash table(s). Although I understand it's not the preference of one reviewer, I've kept with the virObject model. If that still doesn't pass muster and someone else wants to create some other mechanism to combine the existing drivers in a more sane manner, then have at it. This is the model I've chosen. I personally don't see the value in just a shim API. This set of patches moves away from using a strict "uuid/name" designation in favor of using "key1" and "key2". While some may find that "too generic", I think that's the whole purpose of it. After some soul searching I feel using "name" or "uuid" is too restrictive and lends more towards the shim API model. Besides for some consumers they don't have a uuid (nodedev, interface, and nwfilter). In the long run it doesn't matter whether it's a uuid, name, or whatever as long as it's a character string. FWIW: Patches 1, 12, and 16 could be easily separated out, but since I was working in the area - they were added here as well. John Ferlan (17): util: Use VIR_ERROR instead of VIR_WARN util: Introduce virObjectLookupKeys util: Introduce virObjectLookupHash util: Introduce virObjectLookupKeys*Active API's util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind[Locked] util: Introduce virObjectLookupHashForEach util: Introduce virObjectLookupHashSearch[Locked] nodedev: Use virObjectLookup{Keys|Hash} secret: Use virObjectLookup{Keys|Hash} util: Introduce virObjectLookupHashClone Revert "interface: Consume @def in virInterfaceObjNew" interface: Use virObjectLookup{Keys|Hash} test: Clean up test driver Interface interactions util: Introduce virObjectLookupHashPrune network: Fix virNetworkObjBridgeInUse return type network: Use virObjectLookup{Keys|Hash} src/conf/virinterfaceobj.c | 301 ++++++++++---------- src/conf/virnetworkobj.c | 293 ++++++-------------- src/conf/virnetworkobj.h | 5 +- src/conf/virnodedeviceobj.c | 286 ++++++------------- src/conf/virsecretobj.c | 263 +++++------------- src/libvirt_private.syms | 15 + src/test/test_driver.c | 55 +--- src/util/virobject.c | 658 +++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 119 ++++++++ tests/networkxml2conftest.c | 4 +- 10 files changed, 1190 insertions(+), 809 deletions(-) -- 2.9.4
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.4
Add a new virObjectLockable child virObjectLookupKeys that can be used by various driver/vir*obj consumers as the means to define the lookup keys for an object. The API requires that at least the @key1 argument is provided as non-NULL thus ensuring that there's at least one key. The keys will be used as input to a soon to be introduced hash table object that will support one or two hash tables and manage the various add, search, and remove functionality using the keys. The initial consumer will be the driver/vir*obj.c APIs which will make use of the keys via the @UUID or @Name that is used to uniquely describe the object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 18 ++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2149b11..082a0ba 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2318,6 +2318,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupKeys; virClassForObjectRWLockable; virClassIsDerivedFrom; virClassName; @@ -2329,6 +2330,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupKeysNew; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 38db692..aec10eb 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 virObjectLookupKeysClass; static void virObjectLockableDispose(void *anyobj); static void virObjectRWLockableDispose(void *anyobj); +static void virObjectLookupKeysDispose(void *anyobj); static int virObjectOnceInit(void) @@ -93,6 +95,12 @@ virObjectOnceInit(void) virObjectRWLockableDispose))) return -1; + if (!(virObjectLookupKeysClass = virClassNew(virObjectLockableClass, + "virObjectLookupKeys", + sizeof(virObjectLookupKeys), + virObjectLookupKeysDispose))) + return -1; + return 0; } @@ -145,6 +153,21 @@ virClassForObjectRWLockable(void) /** + * virClassForObjectLookupKeys: + * + * Returns the class instance for the virObjectLookupKeys type + */ +virClassPtr +virClassForObjectLookupKeys(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + return virObjectLookupKeysClass; +} + + +/** * virClassNew: * @parent: the parent class * @name: the class name @@ -328,6 +351,68 @@ virObjectRWLockableDispose(void *anyobj) /** + * virObjectLookupKeysNew: + * @klass: the klass to check + * @key1: key to be used for unique identifier (required) + * @key2: second key to be used as secondary unique identifier + * + * Create an object with at least @key1 as a means to provide input + * of an input object to add the object into a hash table. If @key2 is + * provided, then the object will exist into two hash tables for faster + * lookups by key for the table; otherwise, hash table searches would + * need to be used to find data from an object that matches some specific + * search the caller performs. + * + * Returns: New object on success, NULL on failure w/ error message set + */ +void * +virObjectLookupKeysNew(virClassPtr klass, + const char *key1, + const char *key2) +{ + virObjectLookupKeysPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupKeys())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupKeys"), + virClassName(klass)); + return NULL; + } + + if (!key1) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("key1 must be provided")); + return NULL; + } + + if (!(obj = virObjectLockableNew(klass))) + return NULL; + + if (VIR_STRDUP(obj->key1, key1) < 0) + goto error; + + if (VIR_STRDUP(obj->key2, key2) < 0) + goto error; + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupKeysDispose(void *anyobj) +{ + virObjectLookupKeysPtr obj = anyobj; + + VIR_FREE(obj->key1); + VIR_FREE(obj->key2); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -393,7 +478,8 @@ virObjectRef(void *anyobj) static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectLockableClass)) + if (virObjectIsClass(anyobj, virObjectLockableClass) || + virObjectIsClass(anyobj, virObjectLookupKeysClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLockable); diff --git a/src/util/virobject.h b/src/util/virobject.h index ac6cf22..0f7d5ca 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -37,6 +37,9 @@ typedef virObjectLockable *virObjectLockablePtr; typedef struct _virObjectRWLockable virObjectRWLockable; typedef virObjectRWLockable *virObjectRWLockablePtr; +typedef struct _virObjectLookupKeys virObjectLookupKeys; +typedef virObjectLookupKeys *virObjectLookupKeysPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -67,9 +70,18 @@ struct _virObjectRWLockable { virRWLock lock; }; +struct _virObjectLookupKeys { + virObjectLockable parent; + + char *key1; + char *key2; +}; + + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectRWLockable(void); +virClassPtr virClassForObjectLookupKeys(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -120,6 +132,12 @@ void * virObjectRWLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectLookupKeysNew(virClassPtr klass, + const char *key1, + const char *key2) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Using the virObjectRWLockable class as the base with the concepts from virObjectLookupKeys of either 1 or 2 lookup keys possible, create the virObjectLookupHash to manage a common mechanism to store objects for driver/vir*obj.c via self locking hash tables using the RW locking APIs. Each hash table will have: objsKey1 -> Hash table for the LookupKeys->key1 and may also have: objsKey2 -> Hash table for the LookupKeys->key2 A secondary benefit of self locking hash tables is each driver then does not have to keep a higher level driver lock for interactions with the object storage since the object itself can manage locking as needed. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 23 ++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 082a0ba..66cf865 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2318,6 +2318,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupHash; virClassForObjectLookupKeys; virClassForObjectRWLockable; virClassIsDerivedFrom; @@ -2330,6 +2331,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashNew; virObjectLookupKeysNew; virObjectNew; virObjectRef; diff --git a/src/util/virobject.c b/src/util/virobject.c index aec10eb..f677b5f 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -69,10 +69,12 @@ static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectRWLockableClass; static virClassPtr virObjectLookupKeysClass; +static virClassPtr virObjectLookupHashClass; static void virObjectLockableDispose(void *anyobj); static void virObjectRWLockableDispose(void *anyobj); static void virObjectLookupKeysDispose(void *anyobj); +static void virObjectLookupHashDispose(void *anyobj); static int virObjectOnceInit(void) @@ -101,6 +103,12 @@ virObjectOnceInit(void) virObjectLookupKeysDispose))) return -1; + if (!(virObjectLookupHashClass = virClassNew(virObjectRWLockableClass, + "virObjectLookupHash", + sizeof(virObjectLookupHash), + virObjectLookupHashDispose))) + return -1; + return 0; } @@ -168,6 +176,21 @@ virClassForObjectLookupKeys(void) /** + * virClassForObjectLookupHash: + * + * Returns the class instance for the virObjectLookupHash type + */ +virClassPtr +virClassForObjectLookupHash(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + return virObjectLookupHashClass; +} + + +/** * virClassNew: * @parent: the parent class * @name: the class name @@ -413,6 +436,63 @@ virObjectLookupKeysDispose(void *anyobj) /** + * virObjectLookupHashNew: + * @klass: the klass to check + * @tableElemsStart: initial size of each hash table + * @createBoth: boolean to determine how many hash tables to create + * + * Create a new poolable hash table object for storing either 1 or 2 + * hash tables capable of storing virObjectLookupKeys objects. This + * object will use the RWLockable objects in order to allow for concurrent + * table reads by multiple threads looking to return lists of data. + * + * Returns: New object on success, NULL on failure w/ error message set + */ +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart, + bool createBoth) +{ + virObjectLookupHashPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupHash())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupHash"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectRWLockableNew(klass))) + return NULL; + + if (!(obj->objsKey1 = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + if (createBoth && + !(obj->objsKey2 = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupHashDispose(void *anyobj) +{ + virObjectLookupHashPtr obj = anyobj; + + virHashFree(obj->objsKey1); + virHashFree(obj->objsKey2); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -490,7 +570,8 @@ virObjectGetLockableObj(void *anyobj) static virObjectRWLockablePtr virObjectGetRWLockableObj(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectRWLockableClass)) + if (virObjectIsClass(anyobj, virObjectRWLockableClass) || + virObjectIsClass(anyobj, virObjectLookupHashClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectRWLockable); diff --git a/src/util/virobject.h b/src/util/virobject.h index 0f7d5ca..a5c03be 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -23,6 +23,7 @@ # define __VIR_OBJECT_H__ # include "internal.h" +# include "virhash.h" # include "virthread.h" typedef struct _virClass virClass; @@ -40,6 +41,9 @@ typedef virObjectRWLockable *virObjectRWLockablePtr; typedef struct _virObjectLookupKeys virObjectLookupKeys; typedef virObjectLookupKeys *virObjectLookupKeysPtr; +typedef struct _virObjectLookupHash virObjectLookupHash; +typedef virObjectLookupHash *virObjectLookupHashPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -77,11 +81,24 @@ struct _virObjectLookupKeys { char *key2; }; +struct _virObjectLookupHash { + virObjectRWLockable parent; + + /* key1 string -> object mapping for O(1), + * lockless lookup-by-key1 */ + virHashTable *objsKey1; + + /* key2 string -> object mapping for O(1), + * lockless lookup-by-key2 */ + virHashTable *objsKey2; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectRWLockable(void); virClassPtr virClassForObjectLookupKeys(void); +virClassPtr virClassForObjectLookupHash(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -138,6 +155,12 @@ virObjectLookupKeysNew(virClassPtr klass, const char *key2) ATTRIBUTE_NONNULL(1); +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart, + bool createBoth) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Add a 'bool active' field to the virObjectLookupKeys object and then virObjectLookupKeysIsActive and virObjectLookupKeysSetActive API's to manage it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 11 +++++++++++ 3 files changed, 64 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 66cf865..9eb0589 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2332,7 +2332,9 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashNew; +virObjectLookupKeysIsActive; virObjectLookupKeysNew; +virObjectLookupKeysSetActive; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index f677b5f..657597f 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -807,3 +807,54 @@ virObjectListFreeCount(void *list, VIR_FREE(list); } + + +static virObjectLookupKeysPtr +virObjectGetLookupKeysObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupKeysClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLookupKeysClass); + + return NULL; +} + + +/** + * virObjectLookupKeysIsActive + * @anyobj: Pointer to a locked LookupKeys object + * + * Returns: True if object is active, false if not + */ +bool +virObjectLookupKeysIsActive(void *anyobj) +{ + virObjectLookupKeysPtr obj = virObjectGetLookupKeysObj(anyobj); + + if (!obj) + return false; + + return obj->active; +} + + +/** + * virObjectLookupKeysSetActive + * @anyobj: Pointer to a locked LookupKeys object + * @active: New active setting + * + * Set the lookup keys active bool value; value not changed if object + * is not a lookup keys object + */ +void +virObjectLookupKeysSetActive(void *anyobj, + bool active) +{ + virObjectLookupKeysPtr obj = virObjectGetLookupKeysObj(anyobj); + + if (!obj) + return; + + obj->active = active; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index a5c03be..0d3b90b 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -79,6 +79,8 @@ struct _virObjectLookupKeys { char *key1; char *key2; + + bool active; /* true if object is active */ }; struct _virObjectLookupHash { @@ -188,4 +190,13 @@ void virObjectListFreeCount(void *list, size_t count); +bool +virObjectLookupKeysIsActive(void *anyobj) + ATTRIBUTE_NONNULL(1); + +void +virObjectLookupKeysSetActive(void *anyobj, + bool active) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Add a pair of API's to add/remove the LookupKeys object to/from the LookupHash object. The caller must check return status and handle failure properly for the Add. The Remove API callers are all void functions, so only report the problem and ignore the attempt to remove if for some reason the passed @tableobj is not as expected. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 10 ++++++ 3 files changed, 104 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9eb0589..c6d22d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2331,7 +2331,9 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashAdd; virObjectLookupHashNew; +virObjectLookupHashRemove; virObjectLookupKeysIsActive; virObjectLookupKeysNew; virObjectLookupKeysSetActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 657597f..59c8ac6 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -858,3 +858,95 @@ virObjectLookupKeysSetActive(void *anyobj, obj->active = active; } + + +static virObjectLookupHashPtr +virObjectGetLookupHashObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupHashClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_ERROR(anyobj, virObjectLookupHashClass); + + return NULL; +} + + +/** + * virObjectLookupHashAdd: + * @anyobj: LookupHash object + * @obj: The LookupKeys object to insert in the hash table(s) + * + * Insert @obj into the hash tables found in @anyobj. Assumes that the + * caller has determined that the key1 and possibly key2 do not already + * exist in their respective hash table to be inserted. + * + * Returns 0 on success, -1 on failure. + */ +int +virObjectLookupHashAdd(void *anyobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return -1; + + if (obj->key2 && !hashObj->objsKey2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("hashObj=%p has one table, but two keys from obj=%p"), + hashObj, obj); + return -1; + } + + if (virHashAddEntry(hashObj->objsKey1, obj->key1, obj) < 0) + return -1; + virObjectRef(obj); + + if (obj->key2) { + if (virHashAddEntry(hashObj->objsKey2, obj->key2, obj) < 0) { + virHashRemoveEntry(hashObj->objsKey1, obj->key1); + return -1; + } + virObjectRef(obj); + } + + return 0; +} + + +/** + * virObjectLookupHashRemove: + * @anyobj: LookupHash object + * @obj: The LookupKeys object to remove from the hash table(s) + * + * Remove @obj from the hash tables found in @anyobj. The common + * function to remove an object from a hash table will also cause + * the virObjectUnref to be called via virObjectFreeHashData since + * the virHashCreate used that as the Free object element argument. + * + * Even though this is a void, report the error for a bad @anyobj. + */ +void +virObjectLookupHashRemove(void *anyobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj; + + if (!obj) + return; + + if (!(hashObj = virObjectGetLookupHashObj(anyobj))) + return; + + virObjectRef(obj); + virObjectUnlock(obj); + virObjectRWLockWrite(hashObj); + virObjectLock(obj); + virHashRemoveEntry(hashObj->objsKey1, obj->key1); + if (obj->key2) + virHashRemoveEntry(hashObj->objsKey2, obj->key2); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 0d3b90b..bb02781 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -199,4 +199,14 @@ virObjectLookupKeysSetActive(void *anyobj, bool active) ATTRIBUTE_NONNULL(1); +int +virObjectLookupHashAdd(void *anyobj, + virObjectLookupKeysPtr obj) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void +virObjectLookupHashRemove(void *anyobj, + virObjectLookupKeysPtr obj) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
These API's will use the virHashLookup in order to find the object in the hash table 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 | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 10 +++++++ 3 files changed, 81 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c6d22d7..b6ab173 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2332,6 +2332,8 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashFind; +virObjectLookupHashFindLocked; virObjectLookupHashNew; virObjectLookupHashRemove; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 59c8ac6..7dbfd1b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -950,3 +950,72 @@ virObjectLookupHashRemove(void *anyobj, virObjectUnref(obj); virObjectRWUnlock(hashObj); } + + +static virObjectLookupKeysPtr +virObjectLookupHashFindInternal(virObjectLookupHashPtr hashObj, + const char *key) +{ + virObjectLookupKeysPtr obj; + + if ((obj = virHashLookup(hashObj->objsKey1, key))) + return virObjectRef(obj); + + if (hashObj->objsKey2) + obj = virHashLookup(hashObj->objsKey2, key); + + return virObjectRef(obj); +} + + +/** + * virObjectLookupHashFindLocked: + * @anyobj: LookupHash object + * @key: Key to use for lookup + * + * Search through the hash tables looking for the key. The key may be + * either key1 or key2 - 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 + */ +virObjectLookupKeysPtr +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 + */ +virObjectLookupKeysPtr +virObjectLookupHashFind(void *anyobj, + const char *key) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLookupKeysPtr 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 bb02781..dc668ca 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -209,4 +209,14 @@ virObjectLookupHashRemove(void *anyobj, virObjectLookupKeysPtr obj) ATTRIBUTE_NONNULL(1); +virObjectLookupKeysPtr +virObjectLookupHashFindLocked(void *anyobj, + const char *key) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +virObjectLookupKeysPtr +virObjectLookupHashFind(void *anyobj, + const char *key) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Introduce an API to use the virHashForEach API to go through each element of the LookupHash calling a callback with a LookupKey object entry from the LookupHash in order for it to be processed. Create a common structure _virObjectLookupHashForEachData to define the various pieces of data needed by the callback consumers. Fields described in the API function description section. Upon successful completion, the data.nElems is returned to the caller and possibly various fields within the structure filled in depending on the callers need as follows: vir{Object}NumOf{Elem} vir{Object}Get{Key} => Callback function can be combined since generally all that the NumOf cares about is counts, but the Get function also adds some {Key} to a returned list of @names. vir{Object}Export => When @maxElems == -1, will cause allocation into data->elems of an array sized by the hash table size which will be used by the callback function to store addresses of objects specific to each {Object} that are typically associated with the virConnectListAll* API's from the virGet{Object}() calls. The @flags argument is typically used for additional filtering via vir{Object}Match or vir{Object}Filter APIs found in various vir*obj modules. Upon return from the call, the calling function must then move the data->elems into the returned structure. It is up to the callback function to set @error in the event of an error during processing resulting in calling the VIR_FREE() for each of the @nElems already in the array. When an error occurs, any collection done by the callback functions into the @elems array is discarded. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 21 ++++++++++++++ 3 files changed, 96 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b6ab173..3978106 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2334,6 +2334,7 @@ virObjectLockableNew; virObjectLookupHashAdd; virObjectLookupHashFind; virObjectLookupHashFindLocked; +virObjectLookupHashForEach; virObjectLookupHashNew; virObjectLookupHashRemove; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 7dbfd1b..a76f1cb 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1019,3 +1019,77 @@ virObjectLookupHashFind(void *anyobj, return obj; } + + +/** + * virObjectLookupHashForEach + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * For each element of the objsKey1 hash table make a call into the + * callback routine to handle its task. Even if there were two hash + * tables all the objects exist in both, so it's only necessary to + * run through one of them. + * + * NB: + * struct _virObjectLookupHashForEachData { + * virConnectPtr conn; -> Connect ptr for @filter APIs + * void *opaque; -> Opaque data as determined by caller + * void *filter; -> A pointer to function for ACL calls + * bool wantActive; -> Filter active objs + * bool error; -> Set by callback functions for error + * const char *matchStr; -> Filter for specific string in many objs + * unsigned int flags; -> @flags argument to for Export calls + * int nElems; -> # of elements found and passing filters + * void **elems; -> array of elements + * int maxElems; -> maximum # of elements to collect + * Use -1 to allocate array of N table sized + * elements to use for Export functions + * Use -2 for NumOf functions to avoid the + * allocation, but allow sharing with the + * GetNames type functions + * }; + * + * Returns number of elements found on success, -1 on failure + */ +int +virObjectLookupHashForEach(void *anyobj, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return -1; + + if (data->maxElems == -1) { + if (VIR_ALLOC_N(data->elems, virHashSize(hashObj->objsKey1) + 1) < 0) + return -1; + } + + virObjectRWLockRead(hashObj); + virHashForEach(hashObj->objsKey1, callback, data); + virObjectRWUnlock(hashObj); + + if (data->error) + goto error; + + if (data->maxElems == -1) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data->elems, data->nElems + 1)); + } + + return data->nElems; + + error: + if (data->elems) { + if (data->maxElems == -1) { + virObjectListFree(data->elems); + } else { + while (--data->nElems) + VIR_FREE(data->elems[data->nElems]); + } + } + return -1; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index dc668ca..ddebf6c 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -219,4 +219,25 @@ virObjectLookupHashFind(void *anyobj, const char *key) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +typedef struct _virObjectLookupHashForEachData virObjectLookupHashForEachData; +typedef virObjectLookupHashForEachData *virObjectLookupHashForEachDataPtr; +struct _virObjectLookupHashForEachData { + virConnectPtr conn; + void *opaque; + void *filter; + bool wantActive; + bool error; + const char *matchStr; + unsigned int flags; + int nElems; + void **elems; + int maxElems; +}; + +int +virObjectLookupHashForEach(void *anyobj, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
A common object API wrapper 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 LookupKeys object that is represented is returned locked with it's reference count incremented. The virObjectLookupHashSearchLocked 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 | 2 ++ src/util/virobject.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 12 ++++++++ 3 files changed, 88 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3978106..df3f246 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2337,6 +2337,8 @@ virObjectLookupHashFindLocked; virObjectLookupHashForEach; virObjectLookupHashNew; virObjectLookupHashRemove; +virObjectLookupHashSearch; +virObjectLookupHashSearchLocked; virObjectLookupKeysIsActive; virObjectLookupKeysNew; virObjectLookupKeysSetActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index a76f1cb..17ea4e6 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1093,3 +1093,77 @@ virObjectLookupHashForEach(void *anyobj, } return -1; } + + +static virObjectLookupKeysPtr +virObjectLookupHashSearchInternal(virObjectLookupHashPtr hashObj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupKeysPtr obj; + + obj = virHashSearch(hashObj->objsKey1, callback, opaque, NULL); + virObjectRef(obj); + + if (obj) + virObjectLock(obj); + + return obj; +} + + +/** + * virObjectLookupHashSearchLocked + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * Search the hash table objsKey1 table calling the specified @callback + * 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 + */ +virObjectLookupKeysPtr +virObjectLookupHashSearchLocked(void *anyobj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + + if (!hashObj) + return NULL; + + return virObjectLookupHashSearchInternal(hashObj, callback, opaque); +} + + +/** + * virObjectLookupHashSearch + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * Call virObjectLookupHashSearchLocked with a locked hash table + * + * Returns @obj from virObjectLookupHashSearchLocked + */ +virObjectLookupKeysPtr +virObjectLookupHashSearch(void *anyobj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + virObjectLookupKeysPtr obj; + + if (!hashObj) + return NULL; + + virObjectRWLockRead(hashObj); + obj = virObjectLookupHashSearchInternal(hashObj, callback, opaque); + virObjectRWUnlock(hashObj); + + return obj; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index ddebf6c..d4bc3c3 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -240,4 +240,16 @@ virObjectLookupHashForEach(void *anyobj, virObjectLookupHashForEachDataPtr data) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +virObjectLookupKeysPtr +virObjectLookupHashSearchLocked(void *anyobj, + virHashSearcher callback, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +virObjectLookupKeysPtr +virObjectLookupHashSearch(void *anyobj, + virHashSearcher callback, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Use the virObjectLookupKeys in _virNodeDeviceObj and use the virObjectLookupHash in _virNodeDeviceObjList. Convert the code to use the LookupHash object and APIs rather than code within this module that uses virHash* calls. Since the _virNodeDeviceObjList only now contains the @parent object, the virClassNew must be removed from OnceInit because instantiation would fail since the object size would be the same as the parent object size. Usage of HashLookup{Find|Search} APIs returns a locked/reffed object so remove the virObjectLock after FindBy*Locked calls. The only function requiring taking a lock is the Add function since it needs to be synchronized in such a way to avoid multiple threads attempting to add the same named node device at the same time. Also change the virNodeDeviceObjListFilter to follow convention of other drivers of virNodeDeviceObjListACLFilter. The NumOfDevicesCallback and GetNamesCallback can use the same callback function with the only difference being the filling in of the @names array from the passed @data structure if it exists. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 286 +++++++++++++------------------------------- 1 file changed, 80 insertions(+), 206 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b0dcee1..7f025b6 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -34,41 +34,28 @@ VIR_LOG_INIT("conf.virnodedeviceobj"); struct _virNodeDeviceObj { - virObjectLockable parent; + virObjectLookupKeys parent; virNodeDeviceDefPtr def; /* device definition */ }; struct _virNodeDeviceObjList { - virObjectLockable parent; - - /* name string -> virNodeDeviceObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs; - + virObjectLookupHash parent; }; static virClassPtr virNodeDeviceObjClass; -static virClassPtr virNodeDeviceObjListClass; static void virNodeDeviceObjDispose(void *opaque); -static void virNodeDeviceObjListDispose(void *opaque); static int virNodeDeviceObjOnceInit(void) { - if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLockable(), + if (!(virNodeDeviceObjClass = virClassNew(virClassForObjectLookupKeys(), "virNodeDeviceObj", sizeof(virNodeDeviceObj), virNodeDeviceObjDispose))) return -1; - if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(), - "virNodeDeviceObjList", - sizeof(virNodeDeviceObjList), - virNodeDeviceObjListDispose))) - return -1; - return 0; } @@ -85,14 +72,14 @@ virNodeDeviceObjDispose(void *opaque) static virNodeDeviceObjPtr -virNodeDeviceObjNew(void) +virNodeDeviceObjNew(const char *name) { virNodeDeviceObjPtr obj; if (virNodeDeviceObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virNodeDeviceObjClass))) + if (!(obj = virObjectLookupKeysNew(virNodeDeviceObjClass, name, NULL))) return NULL; virObjectLock(obj); @@ -229,17 +216,9 @@ virNodeDeviceObjListSearch(virNodeDeviceObjListPtr devs, virHashSearcher callback, const void *data) { - virNodeDeviceObjPtr obj; - - virObjectLock(devs); - obj = virHashSearch(devs->objs, callback, data, NULL); - virObjectRef(obj); - virObjectUnlock(devs); - - if (obj) - virObjectLock(obj); - - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashSearch(devs, callback, + (void *)data); + return (virNodeDeviceObjPtr) obj; } @@ -274,7 +253,8 @@ static virNodeDeviceObjPtr virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs, const char *name) { - return virObjectRef(virHashLookup(devs->objs, name)); + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(devs, name); + return (virNodeDeviceObjPtr) obj; } @@ -282,15 +262,8 @@ virNodeDeviceObjPtr virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs, const char *name) { - virNodeDeviceObjPtr obj; - - virObjectLock(devs); - obj = virNodeDeviceObjListFindByNameLocked(devs, name); - virObjectUnlock(devs); - if (obj) - virObjectLock(obj); - - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(devs, name); + return (virNodeDeviceObjPtr) obj; } @@ -445,32 +418,10 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs, } -static void -virNodeDeviceObjListDispose(void *obj) -{ - virNodeDeviceObjListPtr devs = obj; - - virHashFree(devs->objs); -} - - virNodeDeviceObjListPtr virNodeDeviceObjListNew(void) { - virNodeDeviceObjListPtr devs; - - if (virNodeDeviceObjInitialize() < 0) - return NULL; - - if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass))) - return NULL; - - if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) { - virObjectUnref(devs); - return NULL; - } - - return devs; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 50, false); } @@ -486,29 +437,31 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def) { virNodeDeviceObjPtr obj; + virNodeDeviceObjPtr ret = NULL; - virObjectLock(devs); + virObjectRWLockWrite(devs); if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) { - virObjectLock(obj); virNodeDeviceDefFree(obj->def); obj->def = def; } else { - if (!(obj = virNodeDeviceObjNew())) + if (!(obj = virNodeDeviceObjNew(def->name))) goto cleanup; - if (virHashAddEntry(devs->objs, def->name, obj) < 0) { - virNodeDeviceObjEndAPI(&obj); + if (virObjectLookupHashAdd(devs, (virObjectLookupKeysPtr)obj) < 0) goto cleanup; - } obj->def = def; virObjectRef(obj); } + ret = obj; + obj = NULL; + cleanup: - virObjectUnlock(devs); - return obj; + virNodeDeviceObjEndAPI(&obj); + virObjectRWUnlock(devs); + return ret; } @@ -516,20 +469,7 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - virNodeDeviceDefPtr def; - - if (!obj) - return; - def = obj->def; - - virObjectRef(obj); - virObjectUnlock(obj); - virObjectLock(devs); - virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); - virObjectUnlock(obj); - virObjectUnref(obj); - virObjectUnlock(devs); + virObjectLookupHashRemove(devs, (virObjectLookupKeysPtr)obj); } @@ -730,29 +670,33 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj, } -struct virNodeDeviceCountData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - const char *matchstr; - int count; -}; - static int -virNodeDeviceObjListNumOfDevicesCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +virNodeDeviceObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { virNodeDeviceObjPtr obj = payload; virNodeDeviceDefPtr def; - struct virNodeDeviceCountData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; virNodeDeviceObjListFilter filter = data->filter; + char **names = (char **)data->elems; + + if (data->error) + return 0; virObjectLock(obj); def = obj->def; + if ((!filter || filter(data->conn, def)) && - (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) - data->count++; + (!data->matchStr || virNodeDeviceObjHasCap(obj, data->matchStr))) { + if (names && VIR_STRDUP(names[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nElems++; + } + cleanup: virObjectUnlock(obj); return 0; } @@ -764,55 +708,11 @@ virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs, const char *cap, virNodeDeviceObjListFilter filter) { - struct virNodeDeviceCountData data = { - .conn = conn, .filter = filter, .matchstr = cap, .count = 0 }; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = NULL, .maxElems = -2 }; - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data); - virObjectUnlock(devs); - - return data.count; -} - - -struct virNodeDeviceGetNamesData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - const char *matchstr; - int nnames; - char **names; - int maxnames; - bool error; -}; - -static int -virNodeDeviceObjListGetNamesCallback(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) -{ - virNodeDeviceObjPtr obj = payload; - virNodeDeviceDefPtr def; - struct virNodeDeviceGetNamesData *data = opaque; - virNodeDeviceObjListFilter filter = data->filter; - - if (data->error) - return 0; - - virObjectLock(obj); - def = obj->def; - - if ((!filter || filter(data->conn, def)) && - (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) { - if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) { - data->error = true; - goto cleanup; - } - data->nnames++; - } - - cleanup: - virObjectUnlock(obj); - return 0; + return virObjectLookupHashForEach(devs, virNodeDeviceObjListGetHelper, &data); } @@ -824,23 +724,11 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, char **const names, int maxnames) { - struct virNodeDeviceGetNamesData data = { - .conn = conn, .filter = filter, .matchstr = cap, .names = names, - .nnames = 0, .maxnames = maxnames, .error = false }; - - virObjectLock(devs); - virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data); - virObjectUnlock(devs); - - if (data.error) - goto error; - - return data.nnames; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .matchStr = cap, + .nElems = 0, .elems = (void **)names, .maxElems = maxnames }; - error: - while (--data.nnames) - VIR_FREE(data.names[data.nnames]); - return -1; + return virObjectLookupHashForEach(devs, virNodeDeviceObjListGetHelper, &data); } @@ -876,15 +764,6 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj, #undef MATCH -struct virNodeDeviceObjListExportData { - virConnectPtr conn; - virNodeDeviceObjListFilter filter; - unsigned int flags; - virNodeDevicePtr *devices; - int ndevices; - bool error; -}; - static int virNodeDeviceObjListExportCallback(void *payload, const void *name ATTRIBUTE_UNUSED, @@ -892,8 +771,10 @@ virNodeDeviceObjListExportCallback(void *payload, { virNodeDeviceObjPtr obj = payload; virNodeDeviceDefPtr def; - struct virNodeDeviceObjListExportData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + virNodeDeviceObjListFilter filter = data->filter; virNodeDevicePtr device = NULL; + virNodeDevicePtr *devices = (virNodeDevicePtr *)data->elems; if (data->error) return 0; @@ -901,19 +782,24 @@ virNodeDeviceObjListExportCallback(void *payload, virObjectLock(obj); def = obj->def; - if ((!data->filter || data->filter(data->conn, def)) && - virNodeDeviceMatch(obj, data->flags)) { - if (data->devices) { - if (!(device = virGetNodeDevice(data->conn, def->name)) || - VIR_STRDUP(device->parent, def->parent) < 0) { - virObjectUnref(device); - data->error = true; - goto cleanup; - } - data->devices[data->ndevices] = device; - } - data->ndevices++; + if (filter && !filter(data->conn, def)) + goto cleanup; + + if (!virNodeDeviceMatch(obj, data->flags)) + goto cleanup; + + if (!devices) { + data->nElems++; + goto cleanup; + } + + if (!(device = virGetNodeDevice(data->conn, def->name)) || + VIR_STRDUP(device->parent, def->parent) < 0) { + virObjectUnref(device); + data->error = true; + goto cleanup; } + devices[data->nElems++] = device; cleanup: virObjectUnlock(obj); @@ -928,31 +814,19 @@ virNodeDeviceObjListExport(virConnectPtr conn, virNodeDeviceObjListFilter filter, unsigned int flags) { - struct virNodeDeviceObjListExportData data = { - .conn = conn, .filter = filter, .flags = flags, - .devices = NULL, .ndevices = 0, .error = false }; - - virObjectLock(devs); - if (devices && - VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) { - virObjectUnlock(devs); - return -1; - } - - virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data); - virObjectUnlock(devs); + int ret; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - if (data.error) - goto cleanup; + if (devices) + data.maxElems = -1; - if (data.devices) { - ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1)); - *devices = data.devices; - } + ret = virObjectLookupHashForEach(devs, virNodeDeviceObjListExportCallback, + &data); - return data.ndevices; + if (devices) + *devices = (virNodeDevicePtr *)data.elems; - cleanup: - virObjectListFree(data.devices); - return -1; + return ret; } -- 2.9.4
Use the virObjectLookupKeys in _virSecretObj and 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 | 263 +++++++++++++----------------------------------- 1 file changed, 68 insertions(+), 195 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 4dca152..0a0e40f 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -38,7 +38,7 @@ VIR_LOG_INIT("conf.virsecretobj"); struct _virSecretObj { - virObjectLockable parent; + virObjectLookupKeys parent; char *configFile; char *base64File; virSecretDefPtr def; @@ -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 { @@ -68,18 +62,12 @@ struct virSecretSearchData { static int virSecretObjOnceInit(void) { - if (!(virSecretObjClass = virClassNew(virClassForObjectLockable(), + if (!(virSecretObjClass = virClassNew(virClassForObjectLookupKeys(), "virSecretObj", sizeof(virSecretObj), virSecretObjDispose))) return -1; - if (!(virSecretObjListClass = virClassNew(virClassForObjectLockable(), - "virSecretObjList", - sizeof(virSecretObjList), - virSecretObjListDispose))) - return -1; - return 0; } @@ -87,14 +75,14 @@ virSecretObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virSecretObj) static virSecretObjPtr -virSecretObjNew(void) +virSecretObjNew(const char *uuidstr) { virSecretObjPtr obj; if (virSecretObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virSecretObjClass))) + if (!(obj = virObjectLookupKeysNew(virSecretObjClass, uuidstr, NULL))) return NULL; virObjectLock(obj); @@ -118,20 +106,7 @@ 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, false); } @@ -151,15 +126,6 @@ virSecretObjDispose(void *opaque) } -static void -virSecretObjListDispose(void *obj) -{ - virSecretObjListPtr secrets = obj; - - virHashFree(secrets->objs); -} - - /** * virSecretObjFindByUUIDLocked: * @secrets: list of secret objects @@ -173,7 +139,8 @@ static virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, const char *uuidstr) { - return virObjectRef(virHashLookup(secrets->objs, uuidstr)); + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(secrets, uuidstr); + return (virSecretObjPtr)obj; } @@ -191,14 +158,9 @@ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, const char *uuidstr) { - virSecretObjPtr obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(secrets, uuidstr); - virObjectLock(secrets); - obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); - virObjectUnlock(secrets); - if (obj) - virObjectLock(obj); - return obj; + return (virSecretObjPtr) obj; } @@ -243,14 +205,12 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr obj = NULL; + virObjectLookupKeysPtr obj = NULL; struct virSecretSearchData data = { .usageType = usageType, .usageID = usageID }; - obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data, NULL); - if (obj) - virObjectRef(obj); - return obj; + obj = virObjectLookupHashSearchLocked(secrets, virSecretObjSearchName, &data); + return (virSecretObjPtr)obj; } @@ -263,6 +223,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,14 +236,12 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr obj; + virObjectLookupKeysPtr obj = NULL; + struct virSecretSearchData data = { .usageType = usageType, + .usageID = usageID }; - virObjectLock(secrets); - obj = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); - virObjectUnlock(secrets); - if (obj) - virObjectLock(obj); - return obj; + obj = virObjectLookupHashSearch(secrets, virSecretObjSearchName, &data); + return (virSecretObjPtr)obj; } @@ -294,23 +258,7 @@ void 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); + virObjectLookupHashRemove(secrets, (virObjectLookupKeysPtr)obj); } @@ -336,7 +284,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virObjectLock(secrets); + virObjectRWLockWrite(secrets); if (oldDef) *oldDef = NULL; @@ -345,7 +293,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 +320,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, @@ -383,7 +329,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, goto cleanup; } - if (!(obj = virSecretObjNew())) + if (!(obj = virSecretObjNew(uuidstr))) goto cleanup; /* Generate the possible configFile and base64File strings @@ -393,11 +339,10 @@ virSecretObjListAdd(virSecretObjListPtr secrets, !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + if (virObjectLookupHashAdd(secrets, (virObjectLookupKeysPtr)obj) < 0) goto cleanup; obj->def = newdef; - virObjectRef(obj); } ret = obj; @@ -405,72 +350,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 +387,9 @@ virSecretObjListGetUUIDsCallback(void *payload, } virUUIDFormat(def->uuid, uuidstr); - data->uuids[data->nuuids++] = uuidstr; + uuids[data->nElems++] = uuidstr; + } else { + data->nElems++; } cleanup: @@ -493,14 +403,11 @@ 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 virObjectLookupHashForEach(secrets, virSecretObjListGetHelper, &data); } @@ -532,22 +439,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 +457,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 +489,21 @@ 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 = virObjectLookupHashForEach(secretobjs, virSecretObjListExportCallback, + &data); + if (secrets) + *secrets = (virSecretPtr *)data.elems; - error: - virObjectListFree(data.secrets); - return -1; + return ret; } @@ -629,23 +514,11 @@ 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 virObjectLookupHashForEach(secrets, virSecretObjListGetHelper, &data); } -- 2.9.4
A convenience API that will utilize the virHashForEach API for the LookupHash in order to create a clone/copy. Primary consumer is the interface driver which has a desire to save off a copy of its only hash table in order to possible restore it if something goes wrong during processing. The callback function's primary purpose is to copy anything within the local LookupKeys into the target. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 9 ++++++ 3 files changed, 89 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index df3f246..c7c9762 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2332,6 +2332,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashClone; virObjectLookupHashFind; virObjectLookupHashFindLocked; virObjectLookupHashForEach; diff --git a/src/util/virobject.c b/src/util/virobject.c index 17ea4e6..0a4195d 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1167,3 +1167,82 @@ virObjectLookupHashSearch(void *anyobj, return obj; } + + +struct cloneData { + virObjectLookupHashCloneCallback callback; + virObjectLookupHashPtr dst; + bool error; +}; + +/* + * Take the provided virHashForEach element and call the @cb function + * with the input @dst hash table and the source element from the + * @src hash table in order to perform the copy - tracking success/ + * failure using the error boolean. + * + * Once there's a failure, no future copy/clone will occur. + * + * The @cb function can expect the @src hash table object to be + * locked upon entry. + * + * Returns 0 to the virHashForEach on success, -1 on failure. + */ +static int +cloneCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virObjectLookupKeysPtr obj = payload; + struct cloneData *data = opaque; + + if (data->error) + return 0; + + virObjectLock(obj); + + if (data->callback(data->dst, obj) < 0) + data->error = true; + + virObjectUnlock(obj); + + if (data->error) + return -1; + + return 0; +} + +/** + * virObjectLookupHashClone + * @srcAnyobj: source LookupHash object to clone from + * @dstAnyobj: destination LookupHash object to clone to + * @cb: callback function to handle the clone + * + * The clone function is designed to traverse each source hash element + * and call the driver specific @cb function with the element from the + * source hash table in order to clone into the destination hash table. + * + * Return 0 on success, -1 on failure + */ +int +virObjectLookupHashClone(void *srcAnyobj, + void *dstAnyobj, + virObjectLookupHashCloneCallback cb) +{ + virObjectLookupHashPtr srcHashObj = virObjectGetLookupHashObj(srcAnyobj); + virObjectLookupHashPtr dstHashObj = virObjectGetLookupHashObj(dstAnyobj); + struct cloneData data = { .callback = cb, .dst = dstHashObj, + .error = false }; + + if (!srcHashObj || !dstHashObj) + return -1; + + virObjectRWLockRead(srcHashObj); + virHashForEach(srcHashObj->objsKey1, cloneCallback, &data); + virObjectRWUnlock(srcHashObj); + + if (data.error) + return -1; + + return 0; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index d4bc3c3..b9e6311 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -252,4 +252,13 @@ virObjectLookupHashSearch(void *anyobj, void *opaque) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +typedef int (*virObjectLookupHashCloneCallback)(void *destHashTable, + void *sourceObject); +int +virObjectLookupHashClone(void *srcAnyobj, + void *dstAnyobj, + virObjectLookupHashCloneCallback cb) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + + #endif /* __VIR_OBJECT_H */ -- 2.9.4
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.4
Use the virObjectLookupKeys in _virInterfaceObj and use the virObjectLookupHash in _virInterfaceObjList. Convert the code to use the LookupHash object and APIs rather than the local forward linked list processing. Usage of HashLookup{Find|Search} API's is via a callback mechanism and returns a locked/reffed object. The Clone API will make use of the ForEach functionality in copying whatever is in one HashLookup list into the destination. The only function requiring taking a lock is the AssignDef function since it needs to be synchronized in such a way to avoid multiple threads attempting to add the same named object at the same time. The NumOfInterfaces and GetNames can use the same callback function with the only difference being the filling in of the @names array from the passed @data structure if it exists. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 300 ++++++++++++++++++++++++--------------------- 1 file changed, 157 insertions(+), 143 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index e993b92..f2475b8 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -33,15 +33,13 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { - virObjectLockable parent; + virObjectLookupKeys parent; - bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ }; struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; + virObjectLookupHash parent; }; /* virInterfaceObj manipulation */ @@ -52,7 +50,7 @@ static void virInterfaceObjDispose(void *obj); static int virInterfaceObjOnceInit(void) { - if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLookupKeys(), "virInterfaceObj", sizeof(virInterfaceObj), virInterfaceObjDispose))) @@ -74,14 +72,14 @@ virInterfaceObjDispose(void *opaque) static virInterfaceObjPtr -virInterfaceObjNew(void) +virInterfaceObjNew(const char *name) { virInterfaceObjPtr obj; if (virInterfaceObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + if (!(obj = virObjectLookupKeysNew(virInterfaceObjClass, name, NULL))) return NULL; virObjectLock(obj); @@ -112,7 +110,7 @@ virInterfaceObjGetDef(virInterfaceObjPtr obj) bool virInterfaceObjIsActive(virInterfaceObjPtr obj) { - return obj->active; + return virObjectLookupKeysIsActive(obj); } @@ -120,7 +118,7 @@ void virInterfaceObjSetActive(virInterfaceObjPtr obj, bool active) { - obj->active = active; + virObjectLookupKeysSetActive(obj, active); } @@ -128,11 +126,40 @@ virInterfaceObjSetActive(virInterfaceObjPtr obj, virInterfaceObjListPtr virInterfaceObjListNew(void) { - virInterfaceObjListPtr interfaces; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 10, false); +} - if (VIR_ALLOC(interfaces) < 0) - return NULL; - return interfaces; + +static int +virInterfaceObjListFindByMACStringCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + char **const matches = (char **const)data->elems; + virInterfaceDefPtr def; + int ret = -1; + + if (data->error) + return 0; + + virObjectLock(obj); + def = obj->def; + if (STRCASEEQ(def->mac, data->matchStr)) { + if (data->nElems < data->maxElems) { + if (VIR_STRDUP(matches[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nElems++; + } + } + ret = 0; + + cleanup: + virObjectUnlock(obj); + return ret; } @@ -142,33 +169,22 @@ virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces, char **const matches, int maxmatches) { - size_t i; - int matchct = 0; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (STRCASEEQ(def->mac, mac)) { - if (matchct < maxmatches) { - if (VIR_STRDUP(matches[matchct], def->name) < 0) { - virObjectUnlock(obj); - goto error; - } - matchct++; - } - } - virObjectUnlock(obj); - } - return matchct; + virObjectLookupHashForEachData data = { + .error = false, .matchStr = mac, .nElems = 0, + .elems = (void **)matches, .maxElems = maxmatches }; + + return virObjectLookupHashForEach(interfaces, + virInterfaceObjListFindByMACStringCb, + &data); +} - error: - while (--matchct >= 0) - VIR_FREE(matches[matchct]); - return -1; +static virInterfaceObjPtr +virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces, + const char *name) +{ + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(interfaces, name); + return (virInterfaceObjPtr)obj; } @@ -176,73 +192,73 @@ virInterfaceObjPtr virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, const char *name) { - size_t i; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - - return NULL; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(interfaces, name); + return (virInterfaceObjPtr) obj; } void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) { - size_t i; + virObjectUnref(interfaces); +} + + +struct interfaceCloneData { + const char *primaryKey; + virInterfaceObjListPtr dest; + bool error; +}; + +static int +virInterfaceObjListCloneCb(void *destHashTable, + void *sourceObject) +{ + virInterfaceObjListPtr dest = destHashTable; + virInterfaceObjPtr srcObj = sourceObject; + int ret = -1; + char *xml = NULL; + virInterfaceObjPtr obj; + virInterfaceDefPtr backup = NULL; + + if (!(xml = virInterfaceDefFormat(srcObj->def))) + goto cleanup; - for (i = 0; i < interfaces->count; i++) - virObjectUnref(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - VIR_FREE(interfaces); + if (!(backup = virInterfaceDefParseString(xml))) + goto cleanup; + + if (!(obj = virInterfaceObjListAssignDef(dest, backup))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(xml); + virInterfaceDefFree(backup); + + return ret; } virInterfaceObjListPtr virInterfaceObjListClone(virInterfaceObjListPtr interfaces) { - size_t i; - unsigned int cnt; - virInterfaceObjListPtr dest; + virInterfaceObjListPtr destInterfaces = NULL; if (!interfaces) return NULL; - if (!(dest = virInterfaceObjListNew())) + if (!(destInterfaces = virInterfaceObjListNew())) return NULL; - cnt = interfaces->count; - for (i = 0; i < cnt; i++) { - virInterfaceObjPtr srcobj = interfaces->objs[i]; - virInterfaceDefPtr backup; - virInterfaceObjPtr obj; - char *xml = virInterfaceDefFormat(srcobj->def); + if (virObjectLookupHashClone(interfaces, destInterfaces, + virInterfaceObjListCloneCb) < 0) + goto cleanup; - if (!xml) - goto error; - - if (!(backup = virInterfaceDefParseString(xml))) { - VIR_FREE(xml); - goto error; - } - - VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) - goto error; - virInterfaceObjEndAPI(&obj); - } + return destInterfaces; - return dest; - - error: - virInterfaceObjListFree(dest); + cleanup: + virObjectUnref(destInterfaces); return NULL; } @@ -252,24 +268,29 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { virInterfaceObjPtr obj; + virInterfaceObjPtr ret = NULL; + + virObjectRWLockWrite(interfaces); - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { virInterfaceDefFree(obj->def); obj->def = def; + } else { + if (!(obj = virInterfaceObjNew(def->name))) + goto cleanup; - return obj; + if (virObjectLookupHashAdd(interfaces, (virObjectLookupKeysPtr)obj) < 0) + goto cleanup; + obj->def = def; } - if (!(obj = virInterfaceObjNew())) - return NULL; + ret = obj; + obj = NULL; - if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - virInterfaceObjEndAPI(&obj); - return NULL; - } - obj->def = def; - return virObjectRef(obj); + cleanup: + virInterfaceObjEndAPI(&obj); + virObjectRWUnlock(interfaces); + return ret; } @@ -277,20 +298,39 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i; + virObjectLookupHashRemove(interfaces, (virObjectLookupKeysPtr)obj); +} - virObjectUnlock(obj); - for (i = 0; i < interfaces->count; i++) { - virObjectLock(interfaces->objs[i]); - if (interfaces->objs[i] == obj) { - virObjectUnlock(interfaces->objs[i]); - virObjectUnref(interfaces->objs[i]); - - VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); - break; + +static int +virInterfaceObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + char **const names = (char **const)data->elems; + virInterfaceDefPtr def; + + if (data->error) + return 0; + + if (data->maxElems >= 0 && data->nElems == data->maxElems) + return 0; + + virObjectLock(obj); + def = obj->def; + if (data->wantActive == virInterfaceObjIsActive(obj)) { + if (names && VIR_STRDUP(names[data->nElems], def->name) < 0) { + data->error = true; + goto cleanup; } - virObjectUnlock(interfaces->objs[i]); - } + data->nElems++; + } + + cleanup: + virObjectUnlock(obj); + return 0; } @@ -298,18 +338,12 @@ int virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, bool wantActive) { - size_t i; - int ninterfaces = 0; - - for (i = 0; (i < interfaces->count); i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virObjectLock(obj); - if (wantActive == virInterfaceObjIsActive(obj)) - ninterfaces++; - virObjectUnlock(obj); - } + virObjectLookupHashForEachData data = { + .wantActive = wantActive, .error = false, .nElems = 0, + .elems = NULL, .maxElems = -2 }; - return ninterfaces; + return virObjectLookupHashForEach(interfaces, virInterfaceObjListGetHelper, + &data); } @@ -319,30 +353,10 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces, char **const names, int maxnames) { - int nnames = 0; - size_t i; - - for (i = 0; i < interfaces->count && nnames < maxnames; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; - - virObjectLock(obj); - def = obj->def; - if (wantActive == virInterfaceObjIsActive(obj)) { - if (VIR_STRDUP(names[nnames], def->name) < 0) { - virObjectUnlock(obj); - goto failure; - } - nnames++; - } - virObjectUnlock(obj); - } - - return nnames; - - failure: - while (--nnames >= 0) - VIR_FREE(names[nnames]); + virObjectLookupHashForEachData data = { + .wantActive = wantActive, .error = false, .nElems = 0, + .elems = (void **)names, .maxElems = maxnames }; - return -1; + return virObjectLookupHashForEach(interfaces, virInterfaceObjListGetHelper, + &data); } -- 2.9.4
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.4
A convenience API that will utilize the virHashForEach API for the LookupHash in order to remove elements from the hash table(s) that match some requirement from the callback. NB: Once elements are removed from objsKey1 - if something goes wrong in objsKey2, the best that can be done is to issue a warning and hope for the best. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 5 +++++ 3 files changed, 42 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7c9762..47393c6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2337,6 +2337,7 @@ virObjectLookupHashFind; virObjectLookupHashFindLocked; virObjectLookupHashForEach; virObjectLookupHashNew; +virObjectLookupHashPrune; virObjectLookupHashRemove; virObjectLookupHashSearch; virObjectLookupHashSearchLocked; diff --git a/src/util/virobject.c b/src/util/virobject.c index 0a4195d..47787f7 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -1246,3 +1246,39 @@ virObjectLookupHashClone(void *srcAnyobj, return 0; } + + +/** + * virObjectLookupHashPrune + * @anyobj: LookupHash object + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * Call the callback function from virHashRemoveSet in order to determine + * if the incoming opaque data should be removed from the hash table(s) + * for the LookupHash. If a second hash table exists and we fail to remove + * data from it + */ +void +virObjectLookupHashPrune(void *anyobj, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(anyobj); + ssize_t cntKey1; + ssize_t cntKey2; + + if (!hashObj) + return; + + virObjectRWLockWrite(hashObj); + cntKey1 = virHashRemoveSet(hashObj->objsKey1, callback, opaque); + if (cntKey1 > 0 && hashObj->objsKey2) { + cntKey2 = virHashRemoveSet(hashObj->objsKey2, callback, opaque); + + if (cntKey2 != cntKey1) + VIR_ERROR(_("removed %zd elems from objsKey1 and %zd elems " + "from objsKey2"), cntKey1, cntKey2); + } + virObjectRWUnlock(hashObj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index b9e6311..54eff81 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -260,5 +260,10 @@ virObjectLookupHashClone(void *srcAnyobj, virObjectLookupHashCloneCallback cb) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void +virObjectLookupHashPrune(void *anyobj, + virHashSearcher callback, + void *opaque) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* __VIR_OBJECT_H */ -- 2.9.4
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.4
Use the virObjectLookupKeys in _virNetworkObj and use the virObjectLookupHash in _virNetworkObjList. Convert the code to use the LookupHash object and APIs rather than the local code and usage of virHash* calls. Since the _virNetworkObjList only contains the @parent object the virClassNew must be removed from OnceInit because instantiation would fail since the object size would be the same as the parent object size. Usage of HashLookup{Find|Search} API's returns a locked/reffed object so need to remove virObjectLock after FindBy*Locked calls. Use the def->name as a second key to the LookupHash to make for faster lookup's by name (instead of using Search on uuid key and matching the name. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnetworkobj.c | 291 ++++++++++++-------------------------------- src/conf/virnetworkobj.h | 3 +- tests/networkxml2conftest.c | 4 +- 3 files changed, 86 insertions(+), 212 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 20f846d..d175191 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -40,11 +40,10 @@ VIR_LOG_INIT("conf.virnetworkobj"); #define INIT_CLASS_ID_BITMAP_SIZE (1<<4) struct _virNetworkObj { - virObjectLockable parent; + virObjectLookupKeys parent; pid_t dnsmasqPid; pid_t radvdPid; - bool active; bool autostart; bool persistent; @@ -61,30 +60,21 @@ struct _virNetworkObj { }; struct _virNetworkObjList { - virObjectLockable parent; - - virHashTablePtr objs; + virObjectLookupHash parent; }; static virClassPtr virNetworkObjClass; -static virClassPtr virNetworkObjListClass; static void virNetworkObjDispose(void *obj); -static void virNetworkObjListDispose(void *obj); static int virNetworkObjOnceInit(void) { - if (!(virNetworkObjClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjClass = virClassNew(virClassForObjectLookupKeys(), "virNetworkObj", sizeof(virNetworkObj), virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), - "virNetworkObjList", - sizeof(virNetworkObjList), - virNetworkObjListDispose))) - return -1; return 0; } @@ -92,14 +82,15 @@ virNetworkObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetworkObj) virNetworkObjPtr -virNetworkObjNew(void) +virNetworkObjNew(const char *uuidstr, + const char *name) { virNetworkObjPtr obj; if (virNetworkObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virNetworkObjClass))) + if (!(obj = virObjectLookupKeysNew(virNetworkObjClass, uuidstr, name))) return NULL; if (!(obj->classIdMap = virBitmapNew(INIT_CLASS_ID_BITMAP_SIZE))) @@ -158,7 +149,7 @@ virNetworkObjGetNewDef(virNetworkObjPtr obj) bool virNetworkObjIsActive(virNetworkObjPtr obj) { - return obj->active; + return virObjectLookupKeysIsActive(obj); } @@ -166,7 +157,7 @@ void virNetworkObjSetActive(virNetworkObjPtr obj, bool active) { - obj->active = active; + virObjectLookupKeysSetActive(obj, active); } @@ -332,36 +323,16 @@ virNetworkObjMacMgrDel(virNetworkObjPtr obj, virNetworkObjListPtr virNetworkObjListNew(void) { - virNetworkObjListPtr nets; - - if (virNetworkObjInitialize() < 0) - return NULL; - - if (!(nets = virObjectLockableNew(virNetworkObjListClass))) - return NULL; - - if (!(nets->objs = virHashCreate(50, virObjectFreeHashData))) { - virObjectUnref(nets); - return NULL; - } - - return nets; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 50, true); } static virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, - const unsigned char *uuid) + const char *uuidstr) { - virNetworkObjPtr obj = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(uuid, uuidstr); - - obj = virHashLookup(nets->objs, uuidstr); - if (obj) - virObjectRef(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(nets, uuidstr); + return (virNetworkObjPtr)obj; } @@ -379,30 +350,11 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { - virNetworkObjPtr obj; - - virObjectLock(nets); - obj = virNetworkObjFindByUUIDLocked(nets, uuid); - virObjectUnlock(nets); - if (obj) - virObjectLock(obj); - return obj; -} - - -static int -virNetworkObjSearchName(const void *payload, - const void *name ATTRIBUTE_UNUSED, - const void *data) -{ - virNetworkObjPtr obj = (virNetworkObjPtr) payload; - int want = 0; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(uuid, uuidstr); - virObjectLock(obj); - if (STREQ(obj->def->name, (const char *)data)) - want = 1; - virObjectUnlock(obj); - return want; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(nets, uuidstr); + return (virNetworkObjPtr)obj; } @@ -410,12 +362,8 @@ static virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj = NULL; - - obj = virHashSearch(nets->objs, virNetworkObjSearchName, name, NULL); - if (obj) - virObjectRef(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFindLocked(nets, name); + return (virNetworkObjPtr)obj; } @@ -433,14 +381,8 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { - virNetworkObjPtr obj; - - virObjectLock(nets); - obj = virNetworkObjFindByNameLocked(nets, name); - virObjectUnlock(nets); - if (obj) - virObjectLock(obj); - return obj; + virObjectLookupKeysPtr obj = virObjectLookupHashFind(nets, name); + return (virNetworkObjPtr)obj; } @@ -470,15 +412,6 @@ virNetworkObjDispose(void *opaque) } -static void -virNetworkObjListDispose(void *opaque) -{ - virNetworkObjListPtr nets = opaque; - - virHashFree(nets->objs); -} - - /* * virNetworkObjUpdateAssignDef: * @network: the network object to update @@ -560,12 +493,12 @@ virNetworkObjAssignDefLocked(virNetworkObjListPtr nets, virNetworkObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(def->uuid, uuidstr); /* See if a network with matching UUID already exists */ - if ((obj = virNetworkObjFindByUUIDLocked(nets, def->uuid))) { + if ((obj = virNetworkObjFindByUUIDLocked(nets, uuidstr))) { virObjectLock(obj); /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(obj->def->name, def->name)) { - virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' is already defined with uuid %s"), obj->def->name, uuidstr); @@ -588,20 +521,17 @@ virNetworkObjAssignDefLocked(virNetworkObjListPtr nets, /* UUID does not match, but if a name matches, refuse it */ if ((obj = virNetworkObjFindByNameLocked(nets, def->name))) { virObjectLock(obj); - virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' already exists with uuid %s"), def->name, uuidstr); goto cleanup; } - if (!(obj = virNetworkObjNew())) - goto cleanup; + if (!(obj = virNetworkObjNew(uuidstr, def->name))) + goto cleanup; - virUUIDFormat(def->uuid, uuidstr); - if (virHashAddEntry(nets->objs, uuidstr, obj) < 0) + if (virObjectLookupHashAdd(nets, (virObjectLookupKeysPtr)obj) < 0) goto cleanup; - virObjectRef(obj); obj->def = def; obj->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE); @@ -638,9 +568,9 @@ virNetworkObjAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockWrite(nets); obj = virNetworkObjAssignDefLocked(nets, def, flags); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return obj; } @@ -784,16 +714,7 @@ void virNetworkObjRemoveInactive(virNetworkObjListPtr nets, virNetworkObjPtr obj) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(obj->def->uuid, uuidstr); - virObjectRef(obj); - virObjectUnlock(obj); - virObjectLock(nets); - virObjectLock(obj); - virHashRemoveEntry(nets->objs, uuidstr); - virObjectUnlock(nets); - virObjectUnref(obj); + virObjectLookupHashRemove(nets, (virObjectLookupKeysPtr)obj); } @@ -968,7 +889,8 @@ virNetworkLoadState(virNetworkObjListPtr nets, obj->floor_sum = floor_sum_val; obj->taint = taint; - obj->active = true; /* network with a state file is by definition active */ + /* network with a state file is by definition active */ + virNetworkObjSetActive(obj, true); cleanup: VIR_FREE(configFile); @@ -1177,14 +1099,15 @@ virNetworkObjBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { + bool ret; virNetworkObjPtr obj; struct virNetworkObjBridgeInUseHelperData data = {bridge, skipname}; - virObjectLock(nets); - obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data, NULL); - virObjectUnlock(nets); + obj = (virNetworkObjPtr)virObjectLookupHashSearch(nets, virNetworkObjBridgeInUseHelper, &data); - return obj != NULL; + ret = obj != NULL; + virNetworkObjEndAPI(&obj); + return ret; } @@ -1309,21 +1232,14 @@ virNetworkMatch(virNetworkObjPtr obj, #undef MATCH -struct virNetworkObjListData { - virConnectPtr conn; - virNetworkPtr *nets; - virNetworkObjListFilter filter; - unsigned int flags; - int nnets; - bool error; -}; - static int -virNetworkObjListPopulate(void *payload, +virNetworkObjListExportCb(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + virNetworkPtr *nets = (virNetworkPtr *)data->elems; + virNetworkObjListFilter filter = data->filter; virNetworkObjPtr obj = payload; virNetworkPtr net = NULL; @@ -1332,15 +1248,14 @@ virNetworkObjListPopulate(void *payload, virObjectLock(obj); - if (data->filter && - !data->filter(data->conn, obj->def)) + if (filter && !filter(data->conn, obj->def)) goto cleanup; if (!virNetworkMatch(obj, data->flags)) goto cleanup; - if (!data->nets) { - data->nnets++; + if (!nets) { + data->nElems++; goto cleanup; } @@ -1349,7 +1264,7 @@ virNetworkObjListPopulate(void *payload, goto cleanup; } - data->nets[data->nnets++] = net; + nets[data->nElems++] = net; cleanup: virObjectUnlock(obj); @@ -1364,34 +1279,19 @@ virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags) { - int ret = -1; - struct virNetworkObjListData data = { - .conn = conn, .nets = NULL, .filter = filter, .flags = flags, - .nnets = 0, .error = false }; - - virObjectLock(netobjs); - if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) - goto cleanup; - - virHashForEach(netobjs->objs, virNetworkObjListPopulate, &data); + int ret; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .error = false, .nElems = 0, + .elems = NULL, .maxElems = 0, .flags = flags }; - if (data.error) - goto cleanup; + if (nets) + data.maxElems = -1; - if (data.nets) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(data.nets, data.nnets + 1)); - *nets = data.nets; - data.nets = NULL; - } + ret = virObjectLookupHashForEach(netobjs, virNetworkObjListExportCb, &data); - ret = data.nnets; - cleanup: - virObjectUnlock(netobjs); - while (data.nets && data.nnets) - virObjectUnref(data.nets[--data.nnets]); + if (nets) + *nets = (virNetworkPtr *)data.elems; - VIR_FREE(data.nets); return ret; } @@ -1407,10 +1307,11 @@ virNetworkObjListForEachHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListForEachHelperData *data = opaque; + virObjectLookupHashForEachDataPtr data = opaque; + struct virNetworkObjListForEachHelperData *helperData = data->opaque; - if (data->callback(payload, data->opaque) < 0) - data->ret = -1; + if (helperData->callback(payload, helperData->opaque) < 0) + helperData->ret = -1; return 0; } @@ -1433,54 +1334,45 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, virNetworkObjListIterator callback, void *opaque) { - struct virNetworkObjListForEachHelperData data = { + struct virNetworkObjListForEachHelperData helperData = { .callback = callback, .opaque = opaque, .ret = 0}; - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); - virObjectUnlock(nets); - return data.ret; -} + virObjectLookupHashForEachData data = { + .opaque = &helperData, .error = false, .maxElems = 0 }; + return virObjectLookupHashForEach(nets, virNetworkObjListForEachHelper, + &data); +} -struct virNetworkObjListGetHelperData { - virConnectPtr conn; - virNetworkObjListFilter filter; - char **names; - int nnames; - int maxnames; - bool active; - bool error; -}; static int virNetworkObjListGetHelper(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) { - struct virNetworkObjListGetHelperData *data = opaque; virNetworkObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + virNetworkObjListFilter filter = data->filter; + char **names = (char **)data->elems; if (data->error) return 0; - if (data->maxnames >= 0 && - data->nnames == data->maxnames) + if (data->maxElems >= 0 && + data->nElems == data->maxElems) return 0; virObjectLock(obj); - if (data->filter && - !data->filter(data->conn, obj->def)) + if (filter && !filter(data->conn, obj->def)) goto cleanup; - if ((data->active && virNetworkObjIsActive(obj)) || - (!data->active && !virNetworkObjIsActive(obj))) { - if (data->names && - VIR_STRDUP(data->names[data->nnames], obj->def->name) < 0) { + if ((data->wantActive && virNetworkObjIsActive(obj)) || + (!data->wantActive && !virNetworkObjIsActive(obj))) { + if (names && VIR_STRDUP(names[data->nElems], obj->def->name) < 0) { data->error = true; goto cleanup; } - data->nnames++; + data->nElems++; } cleanup: @@ -1497,26 +1389,11 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - int ret = -1; - - struct virNetworkObjListGetHelperData data = { - .conn = conn, .filter = filter, .names = names, .nnames = 0, - .maxnames = maxnames, .active = active, .error = false}; - - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); - - if (data.error) - goto cleanup; + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .wantActive = active, .error = false, + .nElems = 0, .elems = (void **)names, .maxElems = maxnames }; - ret = data.nnames; - cleanup: - if (ret < 0) { - while (data.nnames) - VIR_FREE(data.names[--data.nnames]); - } - return ret; + return virObjectLookupHashForEach(nets, virNetworkObjListGetHelper, &data); } @@ -1526,15 +1403,11 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - struct virNetworkObjListGetHelperData data = { - .conn = conn, .filter = filter, .names = NULL, .nnames = 0, - .maxnames = -1, .active = active, .error = false}; - - virObjectLock(nets); - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); + virObjectLookupHashForEachData data = { + .conn = conn, .filter = filter, .wantActive = active, .error = false, + .nElems = 0, .elems = NULL, .maxElems = -2 }; - return data.nnames; + return virObjectLookupHashForEach(nets, virNetworkObjListGetHelper, &data); } @@ -1572,7 +1445,5 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, { struct virNetworkObjListPruneHelperData data = {flags}; - virObjectLock(nets); - virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); - virObjectUnlock(nets); + virObjectLookupHashPrune(nets, virNetworkObjListPruneHelper, &data); } diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index 627277b..050a184 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -28,7 +28,8 @@ typedef struct _virNetworkObj virNetworkObj; typedef virNetworkObj *virNetworkObjPtr; virNetworkObjPtr -virNetworkObjNew(void); +virNetworkObjNew(const char *uuidstr, + const char *name); virNetworkDefPtr virNetworkObjGetDef(virNetworkObjPtr obj); diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 4251a22..1a27aab 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -28,11 +28,13 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr virCommandPtr cmd = NULL; char *pidfile = NULL; dnsmasqContext *dctx = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!(def = virNetworkDefParseFile(inxml))) goto fail; - if (!(obj = virNetworkObjNew())) + virUUIDFormat(def->uuid, uuidstr); + if (!(obj = virNetworkObjNew(uuidstr, def->name))) goto fail; virNetworkObjSetDef(obj, def); -- 2.9.4
On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
[...]
Although I understand it's not the preference of one reviewer, I've kept with the virObject model. If that still doesn't pass muster and someone else wants to create some other mechanism to combine the existing drivers in a more sane manner, then have at it. This is the model I've chosen. I personally don't see the value in just a shim API.
This set of patches moves away from using a strict "uuid/name" designation in favor of using "key1" and "key2". While some may find that "too generic",
Yes. I'm here to complain about this. As I've pointed out some time ago, abstracting stuff, which you can't name properly does not seem to make sense if you are in the end attaching it to something non-generic. This makes such helpers hard to use and hard to remember which fields serves which purpose in the given use case. This also then triggers custom wrappers for every single usage area where you put names to those fields.
I think that's the whole purpose of it. After some soul searching I feel
Could you elaborate please on the purpose and final goal of this. I was't able to piece together what you are trying to achieve. If you want to unify the code that sections of libvirt use to hold lists of objects I don't think you need a custom sub-type for them.
using "name" or "uuid" is too restrictive and lends more towards the shim
It has to be restrictive if you want to couple it to specific objects like VMs and such. It would be too restrictive only if you are going to add a generic implementation of a container holding objects which can be referenced by generic keys. You are trying to add a hybrid. Something which is generic enough to allow anonymous naming, but specific enough that it has to have an "active" field. [1]
API model. Besides for some consumers they don't have a uuid (nodedev, interface, and nwfilter). In the long run it doesn't matter whether it's a uuid, name, or whatever as long as it's a character string.
I don't really see the point in trying to abstract the contents of a "object list". I see value in having a object/set of APIs which would basically allow multiple keys for an entry in a hash table (for any possible implementation of this). All of such can be done on a virObject and you don't need any custom wrapper object which holds certain anonymous properties. The wrapper object you are adding here doesn't seem to add any value, since you can't really remove the name or UUID value from the internal objects, so that still would be duplicated. [...]
John Ferlan (17): util: Use VIR_ERROR instead of VIR_WARN util: Introduce virObjectLookupKeys util: Introduce virObjectLookupHash util: Introduce virObjectLookupKeys*Active API's
[1]
util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind[Locked] util: Introduce virObjectLookupHashForEach util: Introduce virObjectLookupHashSearch[Locked] nodedev: Use virObjectLookup{Keys|Hash} secret: Use virObjectLookup{Keys|Hash} util: Introduce virObjectLookupHashClone Revert "interface: Consume @def in virInterfaceObjNew" interface: Use virObjectLookup{Keys|Hash} test: Clean up test driver Interface interactions util: Introduce virObjectLookupHashPrune network: Fix virNetworkObjBridgeInUse return type network: Use virObjectLookup{Keys|Hash}
On 08/21/2017 09:06 AM, Peter Krempa wrote:
On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
[...]
Although I understand it's not the preference of one reviewer, I've kept with the virObject model. If that still doesn't pass muster and someone else wants to create some other mechanism to combine the existing drivers in a more sane manner, then have at it. This is the model I've chosen. I personally don't see the value in just a shim API.
This set of patches moves away from using a strict "uuid/name" designation in favor of using "key1" and "key2". While some may find that "too generic",
Yes. I'm here to complain about this. As I've pointed out some time ago, abstracting stuff, which you can't name properly does not seem to make sense if you are in the end attaching it to something non-generic.
I suspected that you'd object and that's fine. Oddly it's OK for virhash code to generically call something a "key" and have a default search mechanism that uses a void * STREQ comparison, but yet when applied to virobject code used to combine the needs for the various driver/vir*obj modules to create, add, search, list, remove, and delete for a what amounts to be common code between each of them using hash table(s) to manage "uuid" or "name" based objects, then it's not OK. Ironically my first/initial pass at this took the path of a single src/conf/virpoolobj.c module, but it was suggested to use virobject instead to abstract things. When that's done - it's back to well I don't like that approach because it's too generic, so you should use a common module in order to make all those abstractions along with quite a bit more specific code in order to handle (known) differences between how each of the driver/vir*obj's needs to reference things. FWIW: Initial posting... Patch 2 is where the virobject enhancement was suggested. https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
This makes such helpers hard to use and hard to remember which fields serves which purpose in the given use case. This also then triggers custom wrappers for every single usage area where you put names to those fields.
How is it hard to use/remember? I would think it's much harder to use and remember 8 different ways to do things as opposed to one, but maybe I'm the only one looking to simplify/unify. If you really look at the code - you'll see for the most part it's a wrapper around various virhash functions with specific needs for driver/vir*obj that includes the refcnt, locking, and smaller subset of driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr. It works for nodedev, secret, network, and interface already as seen in later patches in the series. And again, usage of "key1/key2" vs. "uuid/name" is because: Nodedev uses "name" as it's key Secret uses "uuid" as it's key Network uses "uuid" and (with these patches) "name" as keys Interface uses "name" as it's key Nwfilter uses "name" as it's key Storage Pool uses "uuid" and "name" as keys Storage Volume uses "name" as it's key Domains use "uuid" and "name" as keys. So, is that really that hard to remember? Do you really have a need to remember? Why do you feel it's so important that the object knows it's managing a table of UUIDs or a table of names? The driver/vir*obj implementation essentially wants at least 1 way to store things and possibly 2 so that lookup by a second key is quicker (lookup-by-keyid vs. search all for specific name). Yes, they are one or the other currently. In the long run, it's just a name. The driver/vir*obj.c code can choose which to use and manages it's own mapping. One need only look at the argument in the create/new to know which is which. But, it doesn't matter once you get to the Find/Search code as if there are two tables an object has to be in both tables. So, when using a Lookup function on some string both tables can be searched in order to perform lookup. When it comes to the ForEach or Search type functions, only one table needs to be searched in order to run through all the elements. So rather than need to make the (generic) object code need to check if one table or the other exists, just make at least one table required and define that to be the first table. It's just a design decision.
I think that's the whole purpose of it. After some soul searching I feel
Could you elaborate please on the purpose and final goal of this. I was't able to piece together what you are trying to achieve. If you want to unify the code that sections of libvirt use to hold lists of objects I don't think you need a custom sub-type for them.
As if I haven't already elaborated the purpose and final goal. It's been stated multiple times and I think it's very obvious. The goal is to make things generic enough to be used by various driver/vir*obj modules in order to abstract away or combine alike code. If that's truly not desired that's fine. I think it's far better to have less cut/copy-n-paste code, but not everyone has that same viewpoint. Rather than take a myopic viewpoint of a domain or storage or network - once you consider that each of those subsystems essentially does the same thing - creates an object, stores that object, allows searches on that object, and eventually deletes that object. The details of the object is still handle-able by specific code, but there's much with that object that can and should be generically handled. The "reason" I went down this rabbit hole was some patches posted by Virtuozzo which essentially copied *a lot* from the storage driver in order to make another storage driver specific for their needs. I felt rather than cut-copy-n-paste that we should be able to "combine" a lot of code. As I dug into the code I found a mish-mash of object management throughout the drivers. Mostly forward linked lists, but each set of code just different enough to make things "difficult" as it relates to being able to support/maintain code over time. So rather than have multiple ways to do things, take a common approach so that when you're looking through code you don't have to know the differences between 8 piles of code that all accomplished the same goal, but each in its own unique way. Other than NWFilter and Storage Pool/Vols other code is all using hash tables. The NWFitler patches are onlist, but recursive locking is proving to be a PITA. I have patches to convert storage pools to use hash tables ready to post - I was just running it through avocado testing first but found that test harness is a bit broken. Patches to convert storage vols to use a hash table would be in a followup set. So from a code convergence viewpoint - at least mostly everything is fairly common now, which is good. Still, the domain code while using hash tables doesn't exactly conform to refcnt logic very well (yet). I've been waiting to finish everything else before tackling that.
using "name" or "uuid" is too restrictive and lends more towards the shim
It has to be restrictive if you want to couple it to specific objects like VMs and such. It would be too restrictive only if you are going to add a generic implementation of a container holding objects which can be referenced by generic keys.
You are trying to add a hybrid. Something which is generic enough to allow anonymous naming, but specific enough that it has to have an "active" field. [1]
The active could be removed if it was that objectionable. Still let's face it - it's being written to serve a purpose within the libvirt code and it's not some "generic" for general consumption. The 'active' concept is prevalent in many driver/vir*obj's.
API model. Besides for some consumers they don't have a uuid (nodedev, interface, and nwfilter). In the long run it doesn't matter whether it's a uuid, name, or whatever as long as it's a character string.
I don't really see the point in trying to abstract the contents of a "object list". I see value in having a object/set of APIs which would basically allow multiple keys for an entry in a hash table (for any possible implementation of this). All of such can be done on a virObject and you don't need any custom wrapper object which holds certain anonymous properties.
Like I noted above - ironically I took that path originally, but once encouraged to consider augmenting virObject - I found that to be a better solution. I think the shim idea is going to run into some problems, but I'll be happy to look at code someone else writes that accomplishes that. As I see it there are 4 driver/vir*obj that are ready for such a convergence. I have a suspicion that much of what's done could use what I've done; however, I think there will need to be additional code and switches to handle certain tasks that are more easily handled by what I've done. I'll also be very curious to see what happens when the domain code is considered. Suffice to say IMO there's some AFU'd code there especially as it relates to refcnt and locks.
The wrapper object you are adding here doesn't seem to add any value,
That's your opinion.
since you can't really remove the name or UUID value from the internal objects, so that still would be duplicated.
Not sure what you mean here. Which internal object? Oh and here's the magic word "internal" - that all this is - an internal only mechanism to abstract the way to add, search, remove objects from a table. And like other subsystems we can define (to a degree) what the data and APIs would be needed to manage those objects. John
[...]
John Ferlan (17): util: Use VIR_ERROR instead of VIR_WARN util: Introduce virObjectLookupKeys util: Introduce virObjectLookupHash util: Introduce virObjectLookupKeys*Active API's
[1]
util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind[Locked] util: Introduce virObjectLookupHashForEach util: Introduce virObjectLookupHashSearch[Locked] nodedev: Use virObjectLookup{Keys|Hash} secret: Use virObjectLookup{Keys|Hash} util: Introduce virObjectLookupHashClone Revert "interface: Consume @def in virInterfaceObjNew" interface: Use virObjectLookup{Keys|Hash} test: Clean up test driver Interface interactions util: Introduce virObjectLookupHashPrune network: Fix virNetworkObjBridgeInUse return type network: Use virObjectLookup{Keys|Hash}
On Mon, Aug 21, 2017 at 11:27:17 -0400, John Ferlan wrote:
On 08/21/2017 09:06 AM, Peter Krempa wrote:
On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
[...]
Oddly it's OK for virhash code to generically call something a "key" and have a default search mechanism that uses a void * STREQ comparison, but yet when applied to virobject code used to combine the needs for the
virHash code also stores anything you give it and returns it back. In your approach the code is not generic enough to take a virObject or a void *.
various driver/vir*obj modules to create, add, search, list, remove, and delete for a what amounts to be common code between each of them using hash table(s) to manage "uuid" or "name" based objects, then it's not OK.
No, not entirely. As I've said, if you are going to add a "hash table on steroids", it's fine if you call it anonymously. What is not okay, that you are going to use the new object type and make all (domain, storage, ...) objects inherit from it. I strongly dislike this. Object members need to be clearly named, otherwise it will end up only in confusion. You certainly can add some internal data structure, which can be an object, to hold the data which allow this to work. But making domain objects inherit from it is wrong.
Ironically my first/initial pass at this took the path of a single src/conf/virpoolobj.c module, but it was suggested to use virobject instead to abstract things. When that's done - it's back to well I don't like that approach because it's too generic, so you should use a common module in order to make all those abstractions along with quite a bit more specific code in order to handle (known) differences between how each of the driver/vir*obj's needs to reference things.
Your first implementation was neither generic nor specific. It contained generic parts (anonymized name and UUID) and then specific parts, where you'd have to pass in VM/storage/nw objects. It couldn't really be used for anything else. And this is mostly the same. You have a hybrid, where you need the stored objects to inherit from the object you've created, and then introduce fields which make it specific for this only use case "active". With this you are basically attempting to move an implementation detail of the approach to map multiple keys to a virObject of some type to the object properties themselves, which feels wrong. Especially since the properties you are trying to extract can't be named uniformly for all possible objects inheriting from such object.
FWIW: Initial posting... Patch 2 is where the virobject enhancement was suggested.
https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
This makes such helpers hard to use and hard to remember which fields serves which purpose in the given use case. This also then triggers custom wrappers for every single usage area where you put names to those fields.
How is it hard to use/remember? I would think it's much harder to use and remember 8 different ways to do things as opposed to one, but maybe I'm the only one looking to simplify/unify.
If 'key1' is a name of the VM but 'uuid' for a secret, then it's confusing and hard to remember. Confusing is worse than having different implementation. If you are going to argue that 'key1' should ever be interpreted in the context of the lookup functions, then you made it an implementation detail of the lookup functions thus not requiring everything to inherit from it.
If you really look at the code - you'll see for the most part it's a wrapper around various virhash functions with specific needs for driver/vir*obj that includes the refcnt, locking, and smaller subset of driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr.
Yes. That's what I object to. If
It works for nodedev, secret, network, and interface already as seen in later patches in the series.
And again, usage of "key1/key2" vs. "uuid/name" is because:
Nodedev uses "name" as it's key Secret uses "uuid" as it's key Network uses "uuid" and (with these patches) "name" as keys Interface uses "name" as it's key Nwfilter uses "name" as it's key Storage Pool uses "uuid" and "name" as keys Storage Volume uses "name" as it's key Domains use "uuid" and "name" as keys.
Yes they are different. And that's my point. If you are going to make an object, which will agregate those properties, you need to be able to name them. If you can't, don't try
So, is that really that hard to remember? Do you really have a need to
Yes. Also duplicated. You still have the name in vm->def or pool->def. It bugs me that we store the name in 'def' rather the object itself, but I don't feel like trying to change it. (I tried and gave up few years ago.)
remember? Why do you feel it's so important that the object knows it's managing a table of UUIDs or a table of names? The driver/vir*obj
No that's fine. As long as you don't force us to inherit from it. If you inherit from it, the names need to be meaningful. If you treat the object as an impl. Detail then they don't.
implementation essentially wants at least 1 way to store things and possibly 2 so that lookup by a second key is quicker (lookup-by-keyid vs. search all for specific name). Yes, they are one or the other currently. In the long run, it's just a name.
The driver/vir*obj.c code can choose which to use and manages it's own mapping. One need only look at the argument in the create/new to know which is which. But, it doesn't matter once you get to the Find/Search code as if there are two tables an object has to be in both tables. So, when using a Lookup function on some string both tables can be searched in order to perform lookup. When it comes to the ForEach or Search type functions, only one table needs to be searched in order to run through all the elements. So rather than need to make the (generic) object code need to check if one table or the other exists, just make at least one table required and define that to be the first table. It's just a design decision.
I think that's the whole purpose of it. After some soul searching I feel
Could you elaborate please on the purpose and final goal of this. I was't able to piece together what you are trying to achieve. If you want to unify the code that sections of libvirt use to hold lists of objects I don't think you need a custom sub-type for them.
As if I haven't already elaborated the purpose and final goal. It's been stated multiple times and I think it's very obvious.
The goal is to make things generic enough to be used by various driver/vir*obj modules in order to abstract away or combine alike code. If that's truly not desired that's fine. I think it's far better to have less cut/copy-n-paste code, but not everyone has that same viewpoint.
Rather than take a myopic viewpoint of a domain or storage or network - once you consider that each of those subsystems essentially does the same thing - creates an object, stores that object, allows searches on that object, and eventually deletes that object. The details of the object is still handle-able by specific code, but there's much with that object that can and should be generically handled.
The "reason" I went down this rabbit hole was some patches posted by Virtuozzo which essentially copied *a lot* from the storage driver in order to make another storage driver specific for their needs. I felt rather than cut-copy-n-paste that we should be able to "combine" a lot of code. As I dug into the code I found a mish-mash of object management throughout the drivers. Mostly forward linked lists, but each set of code just different enough to make things "difficult" as it relates to being able to support/maintain code over time. So rather than have multiple ways to do things, take a common approach so that when you're looking through code you don't have to know the differences between 8 piles of code that all accomplished the same goal, but each in its own unique way.
Fair enough. So you want a better way to collect objects in lists. That's a very worthwile goal. [... trimmed the rest, since most points would repeat ... ]
The wrapper object you are adding here doesn't seem to add any value,
That's your opinion.
Well, you techincally asked for it by sending it for review. It would be more worhtwhile to refute it rather than dismiss it. I might have not responded to everything, but this message was rather long. Feel free to reiterate points which you feel I've missed.
On 08/22/2017 11:34 AM, Peter Krempa wrote:
On Mon, Aug 21, 2017 at 11:27:17 -0400, John Ferlan wrote:
On 08/21/2017 09:06 AM, Peter Krempa wrote:
On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
[...]
Oddly it's OK for virhash code to generically call something a "key" and have a default search mechanism that uses a void * STREQ comparison, but yet when applied to virobject code used to combine the needs for the
virHash code also stores anything you give it and returns it back. In your approach the code is not generic enough to take a virObject or a void *.
various driver/vir*obj modules to create, add, search, list, remove, and delete for a what amounts to be common code between each of them using hash table(s) to manage "uuid" or "name" based objects, then it's not OK.
No, not entirely. As I've said, if you are going to add a "hash table on steroids", it's fine if you call it anonymously.
What is not okay, that you are going to use the new object type and make all (domain, storage, ...) objects inherit from it. I strongly dislike this. Object members need to be clearly named, otherwise it will end up only in confusion.
There's a difference between dislike and technically incorrect. In my mind - if {k|K}ey1 was uuid and {k|K}ey2 was name and code was altered to handle whether one exists or not, then there'd still be issues with the approach. The effort expended to make those adjustments and then have some other issue may not be worth it IMO. Still I'd like to find some common ground using objects, but I have a feeling that's just not going to be possible. If LookupKeys were removed and LookupHash changed to rename objsKey1 and objsKey2 to objsUUID and objsName, would that provide a path forward or are you so against any of this that it's pointless to even attempt? Trying to figure a way to constructively move forward.
You certainly can add some internal data structure, which can be an object, to hold the data which allow this to work. But making domain objects inherit from it is wrong.
Ironically my first/initial pass at this took the path of a single src/conf/virpoolobj.c module, but it was suggested to use virobject instead to abstract things. When that's done - it's back to well I don't like that approach because it's too generic, so you should use a common module in order to make all those abstractions along with quite a bit more specific code in order to handle (known) differences between how each of the driver/vir*obj's needs to reference things.
Your first implementation was neither generic nor specific. It contained generic parts (anonymized name and UUID) and then specific parts, where you'd have to pass in VM/storage/nw objects. It couldn't really be used for anything else.
The first one changed all the vir*Obj[List] to be virPoolObj[Table]. It worked for the existing driver/vir*objs implementations. It was to a essentially a proof of concept.
And this is mostly the same. You have a hybrid, where you need the stored objects to inherit from the object you've created, and then introduce fields which make it specific for this only use case "active".
I can remove active - it's not that important. Using fields from the creator is mainly a shortcut. Removing them means adjusting the Add/Remove API's a bit to take parameters. Yes, it's a hybrid with new objects that have specific usages. I'm not expecting other areas to use the objects like virObjectLockable is "reused" in many places. If a new driver/vir*obj is created, then using these objects I figure would make life easier.
With this you are basically attempting to move an implementation detail of the approach to map multiple keys to a virObject of some type to the object properties themselves, which feels wrong. Especially since the properties you are trying to extract can't be named uniformly for all possible objects inheriting from such object.
And the alternative solution is to create a common vircommonobjlist.c module that will essentially do something similar. There would be quite a bit of shimmying going on though in order to FindEach and Search ByUUID or ByName. I haven't put too much thought into that model though since I focused on the initial review comments that pointed in the direction of objects. Just because the object is "special" or "directly" purposed, doesn't invalidate it's usefulness.
FWIW: Initial posting... Patch 2 is where the virobject enhancement was suggested.
https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
This makes such helpers hard to use and hard to remember which fields serves which purpose in the given use case. This also then triggers custom wrappers for every single usage area where you put names to those fields.
How is it hard to use/remember? I would think it's much harder to use and remember 8 different ways to do things as opposed to one, but maybe I'm the only one looking to simplify/unify.
If 'key1' is a name of the VM but 'uuid' for a secret, then it's confusing and hard to remember. Confusing is worse than having different
I don't see it as confusing... What I do find confusing is the multiple layers of domain XML post parse processing and validation via callbacks. I find it easy to map name to key1, but trying to figure out and understand all the rules of processing boggles my mind. Then there's the hidden private data structures on top of that!
implementation. If you are going to argue that 'key1' should ever be interpreted in the context of the lookup functions, then you made it an implementation detail of the lookup functions thus not requiring everything to inherit from it.
So you prefer multiple cut-copy-n-paste functions that when one goes to make a change to say which lock mechanism is used, then one must go modify multiple files making multiple of the same changes. Essentially what Michal would be doing to make rwlocks used for all the hash/list processing instead of modifying one place. Likewise when an issue pops up and is fixed in one place, then it would (theoretically at least) need to be fixed in all places. Of course we all know that's not how it usually happens unless a code reviewer asks, wouldn't this also be a problem elsewhere or if by chance the person making the changes actually does the same change across all drivers that would have the same issue.
If you really look at the code - you'll see for the most part it's a wrapper around various virhash functions with specific needs for driver/vir*obj that includes the refcnt, locking, and smaller subset of driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr.
Yes. That's what I object to. If
But this is where I figured the greatest gain is at least w/r/t code reuse without needlessly adding switches to account for specific data.
It works for nodedev, secret, network, and interface already as seen in later patches in the series.
And again, usage of "key1/key2" vs. "uuid/name" is because:
Nodedev uses "name" as it's key Secret uses "uuid" as it's key Network uses "uuid" and (with these patches) "name" as keys Interface uses "name" as it's key Nwfilter uses "name" as it's key Storage Pool uses "uuid" and "name" as keys Storage Volume uses "name" as it's key Domains use "uuid" and "name" as keys.
Yes they are different. And that's my point. If you are going to make an object, which will agregate those properties, you need to be able to name them. If you can't, don't try
A rose by any other name is still a rose.
So, is that really that hard to remember? Do you really have a need to
Yes. Also duplicated. You still have the name in vm->def or pool->def. It bugs me that we store the name in 'def' rather the object itself, but I don't feel like trying to change it. (I tried and gave up few years ago.)
Hmmm... so I create an object that stores the name and/or uuid and that's a problem? Or did I misinterpret what you're trying to say.
remember? Why do you feel it's so important that the object knows it's managing a table of UUIDs or a table of names? The driver/vir*obj
No that's fine. As long as you don't force us to inherit from it. If you inherit from it, the names need to be meaningful. If you treat the object as an impl. Detail then they don't.
implementation essentially wants at least 1 way to store things and possibly 2 so that lookup by a second key is quicker (lookup-by-keyid vs. search all for specific name). Yes, they are one or the other currently. In the long run, it's just a name.
The driver/vir*obj.c code can choose which to use and manages it's own mapping. One need only look at the argument in the create/new to know which is which. But, it doesn't matter once you get to the Find/Search code as if there are two tables an object has to be in both tables. So, when using a Lookup function on some string both tables can be searched in order to perform lookup. When it comes to the ForEach or Search type functions, only one table needs to be searched in order to run through all the elements. So rather than need to make the (generic) object code need to check if one table or the other exists, just make at least one table required and define that to be the first table. It's just a design decision.
I think that's the whole purpose of it. After some soul searching I feel
Could you elaborate please on the purpose and final goal of this. I was't able to piece together what you are trying to achieve. If you want to unify the code that sections of libvirt use to hold lists of objects I don't think you need a custom sub-type for them.
As if I haven't already elaborated the purpose and final goal. It's been stated multiple times and I think it's very obvious.
The goal is to make things generic enough to be used by various driver/vir*obj modules in order to abstract away or combine alike code. If that's truly not desired that's fine. I think it's far better to have less cut/copy-n-paste code, but not everyone has that same viewpoint.
Rather than take a myopic viewpoint of a domain or storage or network - once you consider that each of those subsystems essentially does the same thing - creates an object, stores that object, allows searches on that object, and eventually deletes that object. The details of the object is still handle-able by specific code, but there's much with that object that can and should be generically handled.
The "reason" I went down this rabbit hole was some patches posted by Virtuozzo which essentially copied *a lot* from the storage driver in order to make another storage driver specific for their needs. I felt rather than cut-copy-n-paste that we should be able to "combine" a lot of code. As I dug into the code I found a mish-mash of object management throughout the drivers. Mostly forward linked lists, but each set of code just different enough to make things "difficult" as it relates to being able to support/maintain code over time. So rather than have multiple ways to do things, take a common approach so that when you're looking through code you don't have to know the differences between 8 piles of code that all accomplished the same goal, but each in its own unique way.
Fair enough. So you want a better way to collect objects in lists. That's a very worthwile goal.
[... trimmed the rest, since most points would repeat ... ]
The wrapper object you are adding here doesn't seem to add any value,
That's your opinion.
Well, you techincally asked for it by sending it for review. It would be more worhtwhile to refute it rather than dismiss it.
I'm merely pointing out my opinion of the value of the code to oppose your opinion. There is value since it's a worthwhile goal. This mechanism is one way to achieve that. John
I might have not responded to everything, but this message was rather long. Feel free to reiterate points which you feel I've missed.
participants (2)
-
John Ferlan -
Peter Krempa