[libvirt] [PATCH v3 00/16] virObject adjustments for common object
v2: https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html Pushed the first two patches from v2 since they were ACK'd Changes in this series... Fixed a couple of nits from former patch 3 & 4, but since they weren't ACK'd they're here again, but now as patch 1 & 2. Patch 3 & 4 are an adjusted version of the former patches 5 & 6. Instead of going with generic @primaryKey and @secondaryKey names, this patch will use @uuid and @name for the field names. Additionally instead of using PoolableHashElement as a name, go with a much shorter LookupKeys. So far the LookupKeys{UUID|Name} API's (patch 4) aren't used and could be dropped if it's felt no future API would need them. Former patches 7 & 8 dealing with the generic @def and @newDef object were tossed away and the rest of the logic I'd be changing for virObject is presented as the remaining (new to reviewers) patches 5 -> 16. The object is named using LookupHash which is a follow-on of the LookupKeys. If someone has better suggestion for names, then please provide suggestions rather than just saying I hate the name! It's not my favorite name, but it does convey what it is and IMO is better than just Element. The patches also include the Interface object changes to illustrate that the changes do work. I've run them through the various "interface" tests from the Avocado VT test suite. I also did something similar for the Secret object in my private branch, but did not include that since there are 8 patches on list waiting to be reviewed first. John Ferlan (16): util: Generate a common internal only error print util: Add safety net of checks to ensure valid object util: Introduce virObjectLookupKeys util: Introduce virObjectLookupKeysGet{UUID|Name} interface: Use virObjectLookupKeys util: Introduce virObjectLookupKeys*Active API's interface: Use virObjectLookupKeys*Active util: Introduce virObjectLookupHash util: Introduce virObjectLoookupHashGet{UUID|Name} util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind util: Introduce virObjectLookupHashForEach util: Introduce virObjectLookupHashSearch util: Introduce virObjectLookupHashClone interface: Use virObjectLookupHash test: Clean up test driver Interface interactions src/conf/virinterfaceobj.c | 332 ++++++++++++++---------- src/libvirt_private.syms | 16 ++ src/test/test_driver.c | 55 +--- src/util/virobject.c | 613 ++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 111 ++++++++ 5 files changed, 938 insertions(+), 189 deletions(-) -- 2.9.4
If virObjectIsClass fails "internally" to virobject.c, create a macro to generate the VIR_WARN describing what the problem is. Also improve the checks and message a bit to indicate which was the failure - whether the obj was NULL or just not the right class Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 34805d3..ff9385b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,6 +47,16 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ + do { \ + virObjectPtr obj = anyobj; \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p (%s) is not a %s instance", \ + anyobj, obj->klass->name, #objclass); \ + } while (0) + static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; @@ -312,14 +322,10 @@ virObjectRef(void *anyobj) static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { - virObjectPtr obj; - if (virObjectIsClass(anyobj, virObjectLockableClass)) return anyobj; - obj = anyobj; - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - anyobj, obj ? obj->klass->name : "(unknown)"); + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockableClass); return NULL; } -- 2.9.4
The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen. So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen. To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many object classes. The check is also moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would be required on the failure path. It is still left up to the caller to handle the failed API calls just as it would be if it passed a NULL opaque pointer anyobj. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index ff9385b..443718b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,14 +47,21 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000)) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ + if (VIR_OBJECT_NOTVALID(obj)) { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p has a bad magic number %X", \ + obj, obj->u.s.magic); \ + } else { \ VIR_WARN("Object %p (%s) is not a %s instance", \ anyobj, obj->klass->name, #objclass); \ + } \ } while (0) static virClassPtr virObjectClass; @@ -153,9 +160,14 @@ virClassNew(virClassPtr parent, goto error; klass->parent = parent; + klass->magic = virAtomicIntInc(&magicCounter); + if (klass->magic > 0xCAFEFFFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("too many object classes defined")); + goto error; + } if (VIR_STRDUP(klass->name, name) < 0) goto error; - klass->magic = virAtomicIntInc(&magicCounter); klass->objectSize = objectSize; klass->dispose = dispose; @@ -272,7 +284,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -311,7 +323,7 @@ virObjectRef(void *anyobj) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return NULL; virAtomicIntInc(&obj->u.s.refs); PROBE(OBJECT_REF, "obj=%p", obj); @@ -389,7 +401,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; return virClassIsDerivedFrom(obj->klass, klass); -- 2.9.4
Add a new virObjectLockable child virObjectLookupKeys which can be used by various driver/vir*object consumers as the means to describe the lookup keys @uuid and/or @name. Either or both keys may be defined/used and it's left up to the object consumer to utilize the lookup keys as it sees fit. Eventually, these will be the keys used to insert the object into the hash table(s) for the driver/vir*object consumer. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 17 +++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471..44d712d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2274,6 +2274,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupKeys; virClassIsDerivedFrom; virClassName; virClassNew; @@ -2284,6 +2285,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupKeysNew; virObjectNew; virObjectRef; virObjectUnlock; diff --git a/src/util/virobject.c b/src/util/virobject.c index 443718b..6908e8d 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -66,8 +66,10 @@ struct _virClass { static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; +static virClassPtr virObjectLookupKeysClass; static void virObjectLockableDispose(void *anyobj); +static void virObjectLookupKeysDispose(void *anyobj); static int virObjectOnceInit(void) @@ -84,6 +86,12 @@ virObjectOnceInit(void) virObjectLockableDispose))) return -1; + if (!(virObjectLookupKeysClass = virClassNew(virObjectLockableClass, + "virObjectLookupKeys", + sizeof(virObjectLookupKeys), + virObjectLookupKeysDispose))) + return -1; + return 0; } @@ -121,6 +129,21 @@ virClassForObjectLockable(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 @@ -268,6 +291,58 @@ virObjectLockableDispose(void *anyobj) } +void * +virObjectLookupKeysNew(virClassPtr klass, + const char *uuid, + const char *name) +{ + virObjectLookupKeysPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupKeys())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupKeys"), + virClassName(klass)); + return NULL; + } + + if (!uuid && !name) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("no key, either 'uuid' or 'name' must be defined")); + return NULL; + } + + if (!(obj = virObjectLockableNew(klass))) + return NULL; + + if (VIR_STRDUP(obj->uuid, uuid) < 0) + goto error; + + if (VIR_STRDUP(obj->name, name) < 0) + goto error; + + VIR_DEBUG("obj=%p, uuid=%s name=%s", + obj, NULLSTR(obj->uuid), NULLSTR(obj->name)); + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupKeysDispose(void *anyobj) +{ + virObjectLookupKeysPtr obj = anyobj; + + VIR_DEBUG("dispose obj=%p", obj); + + VIR_FREE(obj->uuid); + VIR_FREE(obj->name); +} + + /** * virObjectUnref: * @anyobj: any instance of virObjectPtr @@ -334,7 +409,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_WARNING(anyobj, virObjectLockableClass); diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..1cade9d 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -34,6 +34,9 @@ typedef virObject *virObjectPtr; typedef struct _virObjectLockable virObjectLockable; typedef virObjectLockable *virObjectLockablePtr; +typedef struct _virObjectLookupKeys virObjectLookupKeys; +typedef virObjectLookupKeys *virObjectLookupKeysPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -59,9 +62,17 @@ struct _virObjectLockable { virMutex lock; }; +struct _virObjectLookupKeys { + virObjectLockable parent; + + char *uuid; + char *name; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); +virClassPtr virClassForObjectLookupKeys(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -108,6 +119,12 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectLookupKeysNew(virClassPtr klass, + const char *uuid, + const char *name) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Add a pair of accessor API's for the object elements which will return the requested key value from the object to the caller. It is up to the caller to check the returned key value and error if the return value is NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 6 ++++++ 3 files changed, 56 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 44d712d..bfc68eb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2285,6 +2285,8 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupKeysGetName; +virObjectLookupKeysGetUUID; virObjectLookupKeysNew; virObjectNew; virObjectRef; diff --git a/src/util/virobject.c b/src/util/virobject.c index 6908e8d..862d46b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -419,6 +419,18 @@ virObjectGetLockableObj(void *anyobj) } +static virObjectLookupKeysPtr +virObjectGetLookupKeysObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupKeysClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLookupKeysClass); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockablePtr @@ -570,3 +582,39 @@ virObjectListFreeCount(void *list, VIR_FREE(list); } + + +/** + * virObjectLookupKeysGetUUID + * @anyobj: Pointer to a LookupKeys object + * + * Returns: Pointer to the object's uuid key value or possibly NULL + */ +const char * +virObjectLookupKeysGetUUID(void *anyobj) +{ + virObjectLookupKeysPtr obj = virObjectGetLookupKeysObj(anyobj); + + if (!obj) + return NULL; + + return obj->uuid; +} + + +/** + * virObjectLookupKeysGetName + * @anyobj: Pointer to a LookupKeys object + * + * Returns: Pointer to the object's name key value or possibly NULL + */ +const char * +virObjectLookupKeysGetName(void *anyobj) +{ + virObjectLookupKeysPtr obj = virObjectGetLookupKeysObj(anyobj); + + if (!obj) + return NULL; + + return obj->name; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 1cade9d..e77634b 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -140,4 +140,10 @@ void virObjectListFreeCount(void *list, size_t count); +const char * +virObjectLookupKeysGetUUID(void *anyobj); + +const char * +virObjectLookupKeysGetName(void *anyobj); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Use the virObjectLookupKeys rather than virObjectLockable. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 106f232..243ff33 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -33,7 +33,7 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { - virObjectLockable parent; + virObjectLookupKeys parent; bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ @@ -52,7 +52,7 @@ static void virInterfaceObjDispose(void *obj); static int virInterfaceObjOnceInit(void) { - if (!(virInterfaceObjClass = virClassNew(virClassForObjectLockable(), + if (!(virInterfaceObjClass = virClassNew(virClassForObjectLookupKeys(), "virInterfaceObj", sizeof(virInterfaceObj), virInterfaceObjDispose))) @@ -81,7 +81,7 @@ virInterfaceObjNew(virInterfaceDefPtr def) if (virInterfaceObjInitialize() < 0) return NULL; - if (!(obj = virObjectLockableNew(virInterfaceObjClass))) + if (!(obj = virObjectLookupKeysNew(virInterfaceObjClass, NULL, def->name))) return NULL; virObjectLock(obj); -- 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 | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 9 +++++++++ 3 files changed, 50 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bfc68eb..6a87e6b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2287,7 +2287,9 @@ virObjectLock; virObjectLockableNew; virObjectLookupKeysGetName; virObjectLookupKeysGetUUID; +virObjectLookupKeysIsActive; virObjectLookupKeysNew; +virObjectLookupKeysSetActive; virObjectNew; virObjectRef; virObjectUnlock; diff --git a/src/util/virobject.c b/src/util/virobject.c index 862d46b..6e2b222 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -585,6 +585,45 @@ virObjectListFreeCount(void *list, /** + * 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; +} + + +/** * virObjectLookupKeysGetUUID * @anyobj: Pointer to a LookupKeys object * diff --git a/src/util/virobject.h b/src/util/virobject.h index e77634b..9e0ecc1 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -67,6 +67,8 @@ struct _virObjectLookupKeys { char *uuid; char *name; + + bool active; /* true if object is active */ }; @@ -140,6 +142,13 @@ void virObjectListFreeCount(void *list, size_t count); +bool +virObjectLookupKeysIsActive(void *anyobj); + +void +virObjectLookupKeysSetActive(void *anyobj, + bool active); + const char * virObjectLookupKeysGetUUID(void *anyobj); -- 2.9.4
Use the @active definition of the virObjectLookupKeys Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 243ff33..69a716b 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -35,7 +35,6 @@ VIR_LOG_INIT("conf.virinterfaceobj"); struct _virInterfaceObj { virObjectLookupKeys parent; - bool active; /* true if interface is active (up) */ virInterfaceDefPtr def; /* The interface definition */ }; @@ -113,7 +112,7 @@ virInterfaceObjGetDef(virInterfaceObjPtr obj) bool virInterfaceObjIsActive(virInterfaceObjPtr obj) { - return obj->active; + return virObjectLookupKeysIsActive(obj); } @@ -121,7 +120,7 @@ void virInterfaceObjSetActive(virInterfaceObjPtr obj, bool active) { - obj->active = active; + virObjectLookupKeysSetActive(obj, active); } -- 2.9.4
Using the virObjectLockable class as the base and the virObjectLookupKeys uuid and name from the virObjectLookupKeys, create the virObjectLookupHash to manage a common mechanism to store objects for a driver via self locking hash tables. Each hash table will have : tableElemsStart -> # of elements to create the initial table objsUUID -> Hash table for the LookupKeys->UUID objsName -> Hash table for the LookupKeys->name 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 | 85 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 24 ++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a87e6b..655a8f8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2274,6 +2274,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectLookupHash; virClassForObjectLookupKeys; virClassIsDerivedFrom; virClassName; @@ -2285,6 +2286,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashNew; virObjectLookupKeysGetName; virObjectLookupKeysGetUUID; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 6e2b222..79f1fe9 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -67,9 +67,11 @@ struct _virClass { static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectLookupKeysClass; +static virClassPtr virObjectLookupHashClass; static void virObjectLockableDispose(void *anyobj); static void virObjectLookupKeysDispose(void *anyobj); +static void virObjectLookupHashDispose(void *anyobj); static int virObjectOnceInit(void) @@ -92,6 +94,12 @@ virObjectOnceInit(void) virObjectLookupKeysDispose))) return -1; + if (!(virObjectLookupHashClass = virClassNew(virObjectLockableClass, + "virObjectLookupHash", + sizeof(virObjectLookupHash), + virObjectLookupHashDispose))) + return -1; + return 0; } @@ -144,6 +152,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 @@ -344,6 +367,65 @@ virObjectLookupKeysDispose(void *anyobj) /** + * virObjectLookupHashNew: + * @klass: the klass to check + * @tableElemsStart: number of elements initially in the table + * + * Create a new poolable hash table object for storing hash tables capable + * of storing virObjectLookupKeys objects. + * + * Returns: New object on success, NULL on failure w/ error message set + */ +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart) +{ + virObjectLookupHashPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLookupHash())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLookupHash"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectLockableNew(klass))) + return NULL; + + obj->tableElemsStart = tableElemsStart; + + if (!(obj->objsUUID = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + if (!(obj->objsName = virHashCreate(tableElemsStart, + virObjectFreeHashData))) + goto error; + + VIR_DEBUG("obj=%p, elems=%d objsUUID=%p objsName=%p", + obj, tableElemsStart, obj->objsUUID, obj->objsName); + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectLookupHashDispose(void *anyobj) +{ + virObjectLookupHashPtr obj = anyobj; + + VIR_DEBUG("dispose obj=%p", obj); + + virHashFree(obj->objsUUID); + virHashFree(obj->objsName); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -410,7 +492,8 @@ static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { if (virObjectIsClass(anyobj, virObjectLockableClass) || - virObjectIsClass(anyobj, virObjectLookupKeysClass)) + virObjectIsClass(anyobj, virObjectLookupKeysClass) || + virObjectIsClass(anyobj, virObjectLookupHashClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockableClass); diff --git a/src/util/virobject.h b/src/util/virobject.h index 9e0ecc1..3c45182 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -23,6 +23,7 @@ # define __VIR_OBJECT_H__ # include "internal.h" +# include "virhash.h" # include "virthread.h" typedef struct _virClass virClass; @@ -37,6 +38,9 @@ typedef virObjectLockable *virObjectLockablePtr; typedef struct _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, @@ -71,10 +75,25 @@ struct _virObjectLookupKeys { bool active; /* true if object is active */ }; +struct _virObjectLookupHash { + virObjectLockable parent; + + int tableElemsStart; + + /* uuid string -> object mapping for O(1), + * lockless lookup-by-uuid */ + virHashTable *objsUUID; + + /* name key -> object mapping for O(1), + * lockless lookup-by-name */ + virHashTable *objsName; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectLookupKeys(void); +virClassPtr virClassForObjectLookupHash(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -127,6 +146,11 @@ virObjectLookupKeysNew(virClassPtr klass, const char *name) ATTRIBUTE_NONNULL(1); +void * +virObjectLookupHashNew(virClassPtr klass, + int tableElemsStart) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Add a pair of accessor API's for the object elements which will return the requested key value from the object to the caller. It is up to the caller to check the returned key value and error if the return value is NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 6 ++++++ 3 files changed, 56 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 655a8f8..28244ab 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2286,6 +2286,8 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashGetName; +virObjectLookupHashGetUUID; virObjectLookupHashNew; virObjectLookupKeysGetName; virObjectLookupKeysGetUUID; diff --git a/src/util/virobject.c b/src/util/virobject.c index 79f1fe9..949a93d 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -514,6 +514,18 @@ virObjectGetLookupKeysObj(void *anyobj) } +static virObjectLookupHashPtr +virObjectGetLookupHashObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectLookupHashClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLookupHashClass); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockablePtr @@ -740,3 +752,39 @@ virObjectLookupKeysGetName(void *anyobj) return obj->name; } + + +/** + * virObjectLookupHashGetUUID + * @anyobj: Pointer to a LookupHash object + * + * Returns: Pointer to the UUID Hash Table or NULL on failure + */ +virHashTablePtr +virObjectLookupHashGetUUID(void *anyobj) +{ + virObjectLookupHashPtr obj = virObjectGetLookupHashObj(anyobj); + + if (!obj) + return NULL; + + return obj->objsUUID; +} + + +/** + * virObjectLookupHashGetName + * @anyobj: Pointer to a LookupHash object + * + * Returns: Pointer to the Name Hash Table or NULL on failure + */ +virHashTablePtr +virObjectLookupHashGetName(void *anyobj) +{ + virObjectLookupHashPtr obj = virObjectGetLookupHashObj(anyobj); + + if (!obj) + return NULL; + + return obj->objsName; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 3c45182..7da680a 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -179,4 +179,10 @@ virObjectLookupKeysGetUUID(void *anyobj); const char * virObjectLookupKeysGetName(void *anyobj); +virHashTablePtr +virObjectLookupHashGetUUID(void *anyobj); + +virHashTablePtr +virObjectLookupHashGetName(void *anyobj); + #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 | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 8 +++++ 3 files changed, 90 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 28244ab..de986e5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2286,9 +2286,11 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLookupHashAdd; virObjectLookupHashGetName; virObjectLookupHashGetUUID; virObjectLookupHashNew; +virObjectLookupHashRemove; virObjectLookupKeysGetName; virObjectLookupKeysGetUUID; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 949a93d..9443dda 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -755,6 +755,86 @@ virObjectLookupKeysGetName(void *anyobj) /** + * virObjectLookupHashAdd: + * @tableobj: LookupHash object with objsUUID/objsName + * @obj: The LookupKeys object to insert in the hash table(s) + * + * Insert @obj into the hash tables found in @tableobj. + * + * Returns 0 on success, -1 on failure. + */ +int +virObjectLookupHashAdd(void *tableobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(tableobj); + + if (!hashObj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("tableobj=%p is not a lookup hash object"), tableobj); + return -1; + } + + if (obj->uuid) { + if (virHashAddEntry(hashObj->objsUUID, obj->uuid, obj) < 0) + return -1; + virObjectRef(obj); + } + + if (obj->name) { + if (virHashAddEntry(hashObj->objsName, obj->name, obj) < 0) { + virHashRemoveEntry(hashObj->objsUUID, obj->uuid); + return -1; + } + virObjectRef(obj); + } + + return 0; +} + + +/** + * virObjectLookupHashRemove: + * @tableobj: LookupHash object with objsUUID/objsName + * @obj: The LookupKeys object to remove from the hash table(s) + * + * Remove @obj from the hash tables found in @tableobj. 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 @tableobj. + */ +void +virObjectLookupHashRemove(void *tableobj, + virObjectLookupKeysPtr obj) +{ + virObjectLookupHashPtr hashObj; + + if (!obj) + return; + + if (!(hashObj = virObjectGetLookupHashObj(tableobj))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("tableobj=%p is not a lookup hash object"), tableobj); + return; + } + + virObjectRef(obj); + virObjectUnlock(obj); + virObjectLock(tableobj); + virObjectLock(obj); + if (obj->uuid) + virHashRemoveEntry(hashObj->objsUUID, obj->uuid); + if (obj->name) + virHashRemoveEntry(hashObj->objsName, obj->name); + virObjectUnlock(obj); + virObjectUnref(obj); + virObjectUnlock(tableobj); +} + + +/** * virObjectLookupHashGetUUID * @anyobj: Pointer to a LookupHash object * diff --git a/src/util/virobject.h b/src/util/virobject.h index 7da680a..422c81e 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -179,6 +179,14 @@ virObjectLookupKeysGetUUID(void *anyobj); const char * virObjectLookupKeysGetName(void *anyobj); +int +virObjectLookupHashAdd(void *tableobj, + virObjectLookupKeysPtr obj); + +void +virObjectLookupHashRemove(void *tableobj, + virObjectLookupKeysPtr obj); + virHashTablePtr virObjectLookupHashGetUUID(void *anyobj); -- 2.9.4
This API will use the virHashLookup in order to find the object in the requested hash table (via @useUUID) by the specified @key. It is up to the caller to know which table (UUID or Name) to find the data by @key and handle the error or NULL return. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 25 +++++++++++++++++++++++++ src/util/virobject.h | 5 +++++ 3 files changed, 31 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de986e5..68961e7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2287,6 +2287,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashFind; virObjectLookupHashGetName; virObjectLookupHashGetUUID; virObjectLookupHashNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 9443dda..01922b3 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -853,6 +853,31 @@ virObjectLookupHashGetUUID(void *anyobj) /** + * virObjectLookupHashFind: + * @tableobj: poolable hash table pointer find data + * @useUUID: boolean to use objsUUID + * @key: Key to use for lookup + * + * Returns a pointer to the entry or NULL on failure + */ +virObjectLookupKeysPtr +virObjectLookupHashFind(void *tableobj, + bool useUUID, + const char *key) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(tableobj); + + if (!hashObj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + ("tableobj=%p is not a lookup hash object"), tableobj); + return NULL; + } + + return virHashLookup(useUUID ? hashObj->objsUUID : hashObj->objsName, key); +} + + +/** * virObjectLookupHashGetName * @anyobj: Pointer to a LookupHash object * diff --git a/src/util/virobject.h b/src/util/virobject.h index 422c81e..ed99540 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -187,6 +187,11 @@ void virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj); +virObjectLookupKeysPtr +virObjectLookupHashFind(void *tableobj, + bool useUUID, + const char *key); + virHashTablePtr virObjectLookupHashGetUUID(void *anyobj); -- 2.9.4
Introduce an API to use the virHashForEach API to go through each element of a specified LookupHash (via @useUUID) calling a callback with a LookupKey object entry from the LookupHash in order for it to be processed. Create a common structure to define the various pieces of data needed by the callback consumers: struct _virObjectLookupHashForEachData { virConnectPtr conn; -> Connect ptr for @aclfilter APIs void *aclfilter; -> 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 }; 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} => Would typically only care about @nelems based on whether the caller cares about @wantActive. Can also filter results based on the @aclfilter call. vir{Object}Get{Key} => Use the @elems and @maxelems in order to fill in an array of char ** strings based on the {Key} filtering data as necessary from the @aclfilter and possibly the @matchstr. Callers of this function will preallocate the @elems and supply the @maxelems value. The @nelems is used to keep track of the number of elements. 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 assocated with 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 | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 20 ++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 68961e7..f783fec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2288,6 +2288,7 @@ virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; virObjectLookupHashFind; +virObjectLookupHashForEach; virObjectLookupHashGetName; virObjectLookupHashGetUUID; virObjectLookupHashNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 01922b3..8a994f5 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -893,3 +893,64 @@ virObjectLookupHashGetName(void *anyobj) return obj->objsName; } + + +/** + * virObjectLookupHashForEach + * @tableobj: poolable hash table pointer to walk through + * @useUUID: boolean to use objsUUID + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * For each element of the objsName hash table make a call into the + * callback routine to handle its task + * + * Returns 0 on success, -1 on failure + */ +int +virObjectLookupHashForEach(void *tableobj, + bool useUUID, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data) +{ + virObjectLookupHashPtr hashObj = virObjectGetLookupHashObj(tableobj); + virHashTablePtr tbl; + + if (!hashObj) + return -1; + + if (useUUID) + tbl = hashObj->objsUUID; + else + tbl = hashObj->objsName; + + if (data->maxelems == -1) { + if (VIR_ALLOC_N(data->elems, virHashSize(tbl) + 1) < 0) + return -1; + } + + virObjectLock(hashObj); + virHashForEach(tbl, callback, data); + virObjectUnlock(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 ed99540..926c9d3 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -198,4 +198,24 @@ virObjectLookupHashGetUUID(void *anyobj); virHashTablePtr virObjectLookupHashGetName(void *anyobj); +typedef struct _virObjectLookupHashForEachData virObjectLookupHashForEachData; +typedef virObjectLookupHashForEachData *virObjectLookupHashForEachDataPtr; +struct _virObjectLookupHashForEachData { + virConnectPtr conn; + void *aclfilter; + bool wantActive; + bool error; + const char *matchstr; + unsigned int flags; + int nelems; + void **elems; + int maxelems; +}; + +int +virObjectLookupHashForEach(void *tableobj, + bool useUUID, + virHashIterator callback, + virObjectLookupHashForEachDataPtr data); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
A common object API wrapper to use the virHashSearch API to search the specified 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. 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 | 1 + src/util/virobject.c | 38 ++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 6 ++++++ 3 files changed, 45 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f783fec..2c4296a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2293,6 +2293,7 @@ virObjectLookupHashGetName; virObjectLookupHashGetUUID; virObjectLookupHashNew; virObjectLookupHashRemove; +virObjectLookupHashSearch; virObjectLookupKeysGetName; virObjectLookupKeysGetUUID; virObjectLookupKeysIsActive; diff --git a/src/util/virobject.c b/src/util/virobject.c index 8a994f5..60bc5b5 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -954,3 +954,41 @@ virObjectLookupHashForEach(void *tableobj, } return -1; } + + +/** + * virObjectLookupHashSearchName + * @tableobj: poolable hash table pointer to search + * @useUUID: boolean to use objsUUID + * @callback: callback function to handle the object specific checks + * @opaque: callback data + * + * Search the objsName hash table calling the specified @callback routine + * with an object and @opaque data in order to determine whether the searched + * object is represented by the @opaque data. + * + * Returns locked/refcnt incremented object on success, NULL on failure + */ +virObjectLookupKeysPtr +virObjectLookupHashSearch(void *tableobj, + bool useUUID, + virHashSearcher callback, + void *opaque) +{ + virObjectLookupHashPtr tableObj = virObjectGetLookupHashObj(tableobj); + virObjectLookupKeysPtr obj; + + if (!tableObj) + return NULL; + + virObjectLock(tableObj); + obj = virHashSearch(useUUID ? tableObj->objsUUID : tableObj->objsName, + callback, opaque); + virObjectRef(obj); + virObjectUnlock(tableObj); + + if (obj) + virObjectLock(obj); + + return obj; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 926c9d3..7e58e34 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -218,4 +218,10 @@ virObjectLookupHashForEach(void *tableobj, virHashIterator callback, virObjectLookupHashForEachDataPtr data); +virObjectLookupKeysPtr +virObjectLookupHashSearch(void *tableobj, + bool useUUID, + virHashSearcher callback, + void *opaque); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
A convenience API that will utilize the virHashForEach API for a specified LookupHash in order to create a clone/copy. Primary consumer is the interface driver which has a desire to save off a copy of it's @Name LookupHash 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 | 10 ++++++ 3 files changed, 90 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2c4296a..41638f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2287,6 +2287,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectLookupHashAdd; +virObjectLookupHashClone; virObjectLookupHashFind; virObjectLookupHashForEach; virObjectLookupHashGetName; diff --git a/src/util/virobject.c b/src/util/virobject.c index 60bc5b5..9182a2e 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -992,3 +992,82 @@ virObjectLookupHashSearch(void *tableobj, return obj; } + + +struct cloneData { + virObjectLookupHashCloneCallback callback; + virObjectLookupHashPtr dst; + bool error; +}; + +/* + * Take the provided virHashForEach element and call the driver @cb function + * with the input @dstTable and the source element from the @srcTable 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 @srcTable 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 + * @srcTable: source poolable hash table pointer to clone from + * @dstTable: destination poolable hash table pointer to clone to + * @useUUID: Use the objsUUID for clone (or objsName) + * @cb: callback function from driver code to handle the clone + * + * The clone function is designed to traverse each @srcTable hash element + * and call the driver specific @cb function with the element from the + * @srcTable in order to clone into the @dstTable. If @useUUID is true, + * then clone the objsUUID table; otherwise, clone the objsName table. + * + * Return 0 on success, -1 on failure + */ +int +virObjectLookupHashClone(void *srcTable, + void *dstTable, + bool useUUID, + virObjectLookupHashCloneCallback cb) +{ + virObjectLookupHashPtr src = virObjectGetLookupHashObj(srcTable); + virObjectLookupHashPtr dst = virObjectGetLookupHashObj(dstTable); + struct cloneData data = { .callback = cb, .dst = dst, .error = false }; + + if (!src || !dst) + return -1; + + virObjectLock(src); + virHashForEach(useUUID ? src->objsUUID : src->objsName, + cloneCallback, &data); + virObjectUnlock(src); + + if (data.error) + return -1; + + return 0; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 7e58e34..350929d 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -224,4 +224,14 @@ virObjectLookupHashSearch(void *tableobj, virHashSearcher callback, void *opaque); +typedef int (*virObjectLookupHashCloneCallback)(void *dstTable, + void *srcElem); +int +virObjectLookupHashClone(void *srcTable, + void *dstTable, + bool useUUID, + virObjectLookupHashCloneCallback cb) + ATTRIBUTE_NONNULL(4); + + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Convert the code to use the LookupHash object and APIs rather than the local forward linked list processing. While this could have been done function by function (for easier review) - converting to the LookupHash would have required changes to the Add, Remove, Find, and List functions anyway in order to properly search hash tables - so rather than piecemeal those changes only to undo them, just use the object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 321 ++++++++++++++++++++++++++------------------- 1 file changed, 189 insertions(+), 132 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index 69a716b..053a5e5 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -39,8 +39,7 @@ struct _virInterfaceObj { }; struct _virInterfaceObjList { - size_t count; - virInterfaceObjPtr *objs; + virObjectLookupHash parent; }; /* virInterfaceObj manipulation */ @@ -128,11 +127,40 @@ virInterfaceObjSetActive(virInterfaceObjPtr obj, virInterfaceObjListPtr virInterfaceObjListNew(void) { - virInterfaceObjListPtr interfaces; + return virObjectLookupHashNew(virClassForObjectLookupHash(), 10); +} - if (VIR_ALLOC(interfaces) < 0) - return NULL; - return interfaces; + +static int +virInterfaceObjListFindByMACStringCallback(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 +170,23 @@ 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; + virObjectLookupHashForEachData data = { + .error = false, .matchstr = mac, .nelems = 0, + .elems = (void **)matches, .maxelems = maxmatches }; - 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; + return virObjectLookupHashForEach(interfaces, false, + virInterfaceObjListFindByMACStringCallback, + &data); +} - error: - while (--matchct >= 0) - VIR_FREE(matches[matchct]); - return -1; +static virInterfaceObjPtr +virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces, + const char *name) +{ + virObjectLookupKeysPtr obj = virObjectLookupHashFind(interfaces, false, + name); + return virObjectRef((virInterfaceObjPtr)obj); } @@ -176,74 +194,96 @@ virInterfaceObjPtr virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces, const char *name) { - size_t i; - - for (i = 0; i < interfaces->count; i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virInterfaceDefPtr def; + virInterfaceObjPtr obj; + virObjectLock(interfaces); + obj = virInterfaceObjListFindByNameLocked(interfaces, name); + virObjectUnlock(interfaces); + if (obj) virObjectLock(obj); - def = obj->def; - if (STREQ(def->name, name)) - return virObjectRef(obj); - virObjectUnlock(obj); - } - return NULL; + return obj; } void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) { - size_t i; + virObjectUnref(interfaces); +} + + +struct interfaceCloneData { + const char *primaryKey; + virInterfaceObjListPtr dest; + bool error; +}; + +static int +virInterfaceObjListCloneCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr srcobj = payload; + struct interfaceCloneData *data = opaque; + int ret = -1; + char *xml = NULL; + virInterfaceObjPtr obj; + virInterfaceDefPtr backup = NULL; + + if (data->error) + return 0; + + virObjectLock(srcobj); + if (!(xml = virInterfaceDefFormat(srcobj->def))) + goto error; + + if (!(backup = virInterfaceDefParseString(xml))) + goto error; + + if (!(obj = virInterfaceObjListAssignDef(data->dest, backup))) + goto error; - for (i = 0; i < interfaces->count; i++) - virObjectUnref(interfaces->objs[i]); - VIR_FREE(interfaces->objs); - VIR_FREE(interfaces); + virInterfaceObjEndAPI(&obj); + ret = 0; + + cleanup: + VIR_FREE(xml); + virInterfaceDefFree(backup); + + return ret; + + error: + data->error = true; + goto cleanup; } virInterfaceObjListPtr virInterfaceObjListClone(virInterfaceObjListPtr interfaces) { - size_t i; - unsigned int cnt; - virInterfaceObjListPtr dest; + virHashTablePtr objsName; + struct interfaceCloneData data = { .error = false }; if (!interfaces) return NULL; - if (!(dest = virInterfaceObjListNew())) + if (!(data.dest = 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); + virObjectLock(interfaces); + objsName = virObjectLookupHashGetName(interfaces); + virHashForEach(objsName, virInterfaceObjListCloneCallback, &data); + virObjectUnlock(interfaces); - if (!xml) - goto error; + if (data.error) + goto cleanup; - if (!(backup = virInterfaceDefParseString(xml))) { - VIR_FREE(xml); - goto error; - } - - VIR_FREE(xml); - if (!(obj = virInterfaceObjListAssignDef(dest, backup))) - goto error; - virInterfaceObjEndAPI(&obj); - } + return data.dest; - return dest; - - error: - virInterfaceObjListFree(dest); - return NULL; + cleanup: + virObjectUnref(data.dest); + return NULL; } @@ -252,24 +292,31 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, virInterfaceDefPtr def) { virInterfaceObjPtr obj; + virInterfaceObjPtr ret = NULL; + + virObjectLock(interfaces); - if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) { + if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) { + virObjectLock(obj); virInterfaceDefFree(obj->def); obj->def = def; - - return obj; + } else { + if (!(obj = virInterfaceObjNew(def))) + goto cleanup; + + if (virObjectLookupHashAdd(interfaces, + (virObjectLookupKeysPtr)obj) < 0) { + obj->def = NULL; + virInterfaceObjEndAPI(&obj); + goto cleanup; + } } - if (!(obj = virInterfaceObjNew(def))) - return NULL; + ret = obj; - if (VIR_APPEND_ELEMENT_COPY(interfaces->objs, - interfaces->count, obj) < 0) { - obj->def = NULL; - virInterfaceObjEndAPI(&obj); - return NULL; - } - return virObjectRef(obj); + cleanup: + virObjectUnlock(interfaces); + return ret; } @@ -277,20 +324,24 @@ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) { - size_t i; + virObjectLookupHashRemove(interfaces, (virObjectLookupKeysPtr)obj); +} + + +static int +virInterfaceObjListNumOfInterfacesCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virInterfaceObjPtr obj = payload; + virObjectLookupHashForEachDataPtr data = opaque; + virObjectLock(obj); + if (data->wantActive == virInterfaceObjIsActive(obj)) + data->nelems++; 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; - } - virObjectUnlock(interfaces->objs[i]); - } + + return 0; } @@ -298,18 +349,44 @@ int virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces, bool wantActive) { - size_t i; - int ninterfaces = 0; + virObjectLookupHashForEachData data = { + .wantActive = wantActive, .nelems = 0, .error = false }; - for (i = 0; (i < interfaces->count); i++) { - virInterfaceObjPtr obj = interfaces->objs[i]; - virObjectLock(obj); - if (wantActive == virInterfaceObjIsActive(obj)) - ninterfaces++; - virObjectUnlock(obj); - } + return virObjectLookupHashForEach(interfaces, false, + virInterfaceObjListNumOfInterfacesCb, + &data); +} + + +static int +virInterfaceObjListGetNamesCb(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 (VIR_STRDUP(names[data->nelems], def->name) < 0) { + data->error = true; + goto cleanup; + } + data->nelems++; + } - return ninterfaces; + cleanup: + virObjectUnlock(obj); + return 0; } @@ -319,30 +396,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, false, + virInterfaceObjListGetNamesCb, &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 11e7fd8..ff2077c 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); @@ -3630,11 +3631,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); @@ -3647,12 +3644,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); } @@ -3662,14 +3655,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); } @@ -3677,12 +3664,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); } @@ -3692,14 +3675,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); } @@ -3732,12 +3709,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; @@ -3821,6 +3794,7 @@ testInterfaceChangeCommit(virConnectPtr conn, } virInterfaceObjListFree(privconn->backupIfaces); + privconn->backupIfaces = NULL; privconn->transaction_running = false; ret = 0; @@ -3851,8 +3825,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; @@ -3899,11 +3872,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); @@ -3913,7 +3885,6 @@ testInterfaceDefineXML(virConnectPtr conn, cleanup: virInterfaceDefFree(def); virInterfaceObjEndAPI(&obj); - testDriverUnlock(privconn); return ret; } -- 2.9.4
ping? Tks, John On 06/22/2017 10:02 AM, John Ferlan wrote:
v2: https://www.redhat.com/archives/libvir-list/2017-June/msg00070.html
Pushed the first two patches from v2 since they were ACK'd
Changes in this series...
Fixed a couple of nits from former patch 3 & 4, but since they weren't ACK'd they're here again, but now as patch 1 & 2.
Patch 3 & 4 are an adjusted version of the former patches 5 & 6. Instead of going with generic @primaryKey and @secondaryKey names, this patch will use @uuid and @name for the field names. Additionally instead of using PoolableHashElement as a name, go with a much shorter LookupKeys. So far the LookupKeys{UUID|Name} API's (patch 4) aren't used and could be dropped if it's felt no future API would need them.
Former patches 7 & 8 dealing with the generic @def and @newDef object were tossed away and the rest of the logic I'd be changing for virObject is presented as the remaining (new to reviewers) patches 5 -> 16.
The object is named using LookupHash which is a follow-on of the LookupKeys.
If someone has better suggestion for names, then please provide suggestions rather than just saying I hate the name! It's not my favorite name, but it does convey what it is and IMO is better than just Element.
The patches also include the Interface object changes to illustrate that the changes do work. I've run them through the various "interface" tests from the Avocado VT test suite. I also did something similar for the Secret object in my private branch, but did not include that since there are 8 patches on list waiting to be reviewed first.
John Ferlan (16): util: Generate a common internal only error print util: Add safety net of checks to ensure valid object util: Introduce virObjectLookupKeys util: Introduce virObjectLookupKeysGet{UUID|Name} interface: Use virObjectLookupKeys util: Introduce virObjectLookupKeys*Active API's interface: Use virObjectLookupKeys*Active util: Introduce virObjectLookupHash util: Introduce virObjectLoookupHashGet{UUID|Name} util: Introduce virObjectLookupHash{Add|Remove} util: Introduce virObjectLookupHashFind util: Introduce virObjectLookupHashForEach util: Introduce virObjectLookupHashSearch util: Introduce virObjectLookupHashClone interface: Use virObjectLookupHash test: Clean up test driver Interface interactions
src/conf/virinterfaceobj.c | 332 ++++++++++++++---------- src/libvirt_private.syms | 16 ++ src/test/test_driver.c | 55 +--- src/util/virobject.c | 613 ++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 111 ++++++++ 5 files changed, 938 insertions(+), 189 deletions(-)
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote: Let's move the discussion [1] into correct place.
Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name. We don't need the virObjectLookupKeys object at all. For example, current add/remove API is: ==== virObjectLookupHashNew ==== virObjectLookupHashNew(virClassPtr klass, int tableElemsStart); could be virObjectListNew(int tableSize, bool useName, bool useUUID); or typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType; virObjectListNew(int tableSize, unsigned int flags); I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size. ==== virObjectLookupHash(Add|Remove) ==== virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj); virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj); The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name. virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid); virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid); or virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid); Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject. ==== virObjectLookupHashFind ==== virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key); could be split into two APIs: virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID); Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is. ==== virObjectLookupHashForEach ==== virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data); could be virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData); There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do. The @useUUID argument also applies to virObjectLookupHashSearch(). The usage of the virObjectList* APIs could be something like this: virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); } static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); } Pavel
On 07/25/2017 05:29 AM, Pavel Hrdina wrote:
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:
Let's move the discussion [1] into correct place.
Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name.
We don't need the virObjectLookupKeys object at all.
First off - thanks for taking the time to put your thoughts here. It really helped me understand where I believe you were coming from. There are some disadvantages to working at home when it comes to sounding boards or drawing things up on a white board for those around you in an office to provide/receive face-2-face feedback. Black and white text can be harsher than intended. Now that I understand that it's less 'wrong' and more 'not necessary' or as you put it a personal dislike I can come in off of the ledge ;-). Still there's a lot of history w/r/t the initial posting where I was encouraged to use/update Objects vs. doing what I did creating a new module and essentially renaming objs and objlists. Scaling back and considering usage of objects sent me down this current path which hasn't received as much feedback. It may not be 100% correct, but I really was hoping it wasn't wrong especially since I have working code. Storing the UUID/Name in an @obj is the means I use to make certain decisions along the way - especially w/r/t Add/Remove logic which as I pointed out previously - there's no way to "Add" something without a key nor "Remove" it; however, the Lookup/Search logic do not need it because the hash table keeps track of the key and all the find/lookup vir*obj logic would have the @def->{uuid|name} as input for the search. The ForEach logic won't use it, although it could (more later). Storing the keys in @obj removes a few decision points from the caller. The consumer only cares they've created an object with 1 or 2 keys in order to allow the objList code "find" those keys and make decisions based on the key rather than having multiple API's that fetch using the key name in the API name. Adding to the objlist is just that - adding and not caring/knowing how many or which tables the object is using. While it's certainly not necessary to have UUID/Name and a LookupKeys object, it is convenient and more than just for UUID/Name. The new object is (will be) used to add bool's (active, persistent, started, etc.) that are found in multiple objects in order to have a common API to manage that. It's not required, but something possible. There are some other 'shared' type pieces of data (configFile, autostartLink, autostart, etc.) that could also make use of "sharing" data and code if so inclined. Of course the primary reason I've adjusted all those obj->def->X to be def = obj->def and def->X access is that moving the @def and @newDef inside the object was another initial goal, which needs to be rethought/reworked as that was considered too generic and possibly prone to coding errors.
For example, current add/remove API is:
==== virObjectLookupHashNew ====
virObjectLookupHashNew(virClassPtr klass, int tableElemsStart);
could be
virObjectListNew(int tableSize, bool useName, bool useUUID);
or
typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType;
virObjectListNew(int tableSize, unsigned int flags);
I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size.
Well @klass is a parameter for other virObject*New() functions, so why should virObjectLookupHashNew not have one? Or asked differently how would virObjectListNew determine it was derived from some class? Does that leave the hash tables in the calling vir*obj.c files or in some common file, such as vircommonobjlist.c? To me that's just a shim to virObject for each of the vir*obj.c. I don't like that solution - we shouldn't create a shim that doesn't converge objects, we would take the plunge and create object code. I also don't like ObjectList if I'm using a hash table - it's not a representative name. Perhaps I'm missing something else you had in mind. @tableElemsStart is the starting size of the hash table that can grow beyond the starting size. Whether it changes names to @tableSize is just window dressing to hide the bikes in the shed. I think the reason why I used a variable was to allow different sizes for different ObjLists. There may have been something else but that's long since left the short term memory. It might be for the clone or backup copy of an ObjList and wanted to have the original table size in order to create the clone. Doesn't seem I use it now though, so saving it could be dropped. Heck we could drop the initial size logic too and give virHashGrow some exercise. Although I see that has some interesting limitations w/r/t bucket chain length and larger element count tables where a max of 2048 buckets can be created allowing bucket chains to potentially grow really long. Perhaps not a problem today with test beds using 100's of objects, but IIRC there was a storage example using 1000's of volumes if not 10's of 1000's. Once you get to about 15K objs, then the probability of longer chains increases (I played around with virhashtest a bit - which currently only maxes at 250 objs, yawn). FWIW: I went away from @useName or @useBool type logic because some tables have UUID only, some have Name only, others have both. For those with one - there is a waste of space, but if we start with table sizes of 1, it's minimized. Again, I preferred @primary and @secondary, but that idea was already squashed. In any case, here's a crudely drawn table of UUID/Name and the various vir*obj consumers. UUID | Name ____________________|___________ NodeDev No | Yes ____________________|___________ Secret Yes | No [1] ____________________|___________ NWFilter Maybe[2]| Yes ____________________|___________ Interface No | Yes ____________________|___________ Network Yes | Yes ____________________|___________ StoragePool Yes | Yes ____________________|___________ StorageVol No | Yes ____________________|___________ Domain Yes | Yes ____________________|___________ [1] I do have patches that would make the secret usage_id be a secondary key [2] Awful piece of history that allows the nwfilter to be defined without a UUID if saving the UUID to the file fails, thus allowing a future start to overwrite the UUID of a previous nwfilter. There's also the possibility that Snapshots could use this, but I don't think it's as beneficial since "unpredictable ordering" for HashTables could make things AFU'd and I don't suppose you'd end up with 1000's of snapshots.
==== virObjectLookupHash(Add|Remove) ====
virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj);
virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj);
The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name.
virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid);
virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid);
or
virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid);
Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject.
Ahhh.. and this is where our fundamental difference lies. You see obj->uuid and obj->name as unnecessary and I find them useful especially for the purpose of adding/removing @obj to/from the @listObj. It seems you'd rather see virObject* API's (or perhaps some new vircommonobjlist.c) that force the callers to know which call to use (ByName/ByUUID) and which table's were created for the object. I prefer having a common API that doesn't need to be that specific, but does the thing just using an argument. Your characterization of "flexible" is my characterization of "control". It's not like UUID/Name are going away - they're just being managed in a lower layer. My view is that the @obj as created during vir*ObjNew is what decides which tables it will need to be present in. The opposing view is I have an object, I'm going to place it in this table and possibly that table and I know where I want to put it and will manage that from the calling function entirely. One thing I would like to see changed is *Remove returning status. Not a a lot of good happens if something during the Remove processing fails and no one is told.
==== virObjectLookupHashFind ====
virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key);
could be split into two APIs:
virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID);
Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is.
Six of one, half-dozen of the other. The consumer is still going to call one or the other ByName/ByUUID, but that leaves that decision point higher up. It's easy enough to make ByName and ByUUID API's, but they just turn into shims to the common code - so why bother? I still prefer the concept of @primary and @secondary. Consider the table above - should it matter that a table stores UUID's? or Names? or just one or two keys? We made the decision early on during ObjNew what our keys would be. After that it's up to calling function to provide some key that we'll search on. For some consumers which use both tables, they want to Find in a specific table and that's why you'd have a boolean to say - don't search primary, search secondary. How about a different idea. Why not make the hash table count variable (1, 2, 3, etc.) and let the caller decide how many to create and which to use. That way the nodedev code removes the 4 or 5 FindBy type API's and replaces it with 4 or 5 hash tables to allow for lookup by key. The consumer can then have vir*ObjListFind{ByUUID|ByName}, but that ends up being a virObjectLookupHashFind(tableobj, tableidx, const char *key) where the consumer knows tableidx = 0 is UUID, tableidx = 1 is Name, etc.
==== virObjectLookupHashForEach ====
virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data);
could be
virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData);
There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do.
The @useUUID argument also applies to virObjectLookupHashSearch().
[NB: written before rereading and writing the above paragraph regarding configurable number of hash tables] How do you choose which table? How do you know which data the caller wants? Refer to the table above - some objects have UUID and some have Name. Some consumers may even want to return a list of all objects by UUID using an input parameter and other by Name with a different input parameter. The *ForEach ends up being a multi-purpose backend for NumOf, Get{Name|UUID}, and Export functions. Typically the Get{Name|UUID} is the "primary key" for the object, but I'm just saying... While I'm typing this response I just realized that the Get{Names|UUIDs} type API functions wouldn't need vir*obj.c specific *Callback functions to deref obj->def->{name|uuid} since obj->{uuid|name} exists! I'd still need some function to determine whether @aclfilter was passed or not for @def and I'd to know to @useUUID or not. But I could converge things even more than I have.
The usage of the virObjectList* APIs could be something like this:
virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); }
static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); }
Pavel
And here's a perfect example of an object that only lives in one table. If I call virObjectLookupHashForEach or virObjectLookupHashSearch and expect to find something in objsUUID, I'm going to be sorely disappointed. One solution is to have two API's - my solution is one API with a parameter that dictates. Given the amount of change since this series was first posted, I have to deal with merge conflicts, the virHashSearch new argument, and using RW locks for all the objlists. So I'm going to have to repost the patches again anyway. At this time, I prefer to leave them as is for the reasons stated regarding avoiding a shim. If that's the avenue to take, then I'll let someone else go down that path. I can finish up cleanup and conversion of all the vir*obj code to use vir*Obj and vir*ObjList, but converging to a shim is something I'll avoid. John
On Thu, Jul 27, 2017 at 04:46:41PM -0400, John Ferlan wrote:
On 07/25/2017 05:29 AM, Pavel Hrdina wrote:
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:
Let's move the discussion [1] into correct place.
Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name.
We don't need the virObjectLookupKeys object at all.
First off - thanks for taking the time to put your thoughts here. It really helped me understand where I believe you were coming from. There are some disadvantages to working at home when it comes to sounding boards or drawing things up on a white board for those around you in an office to provide/receive face-2-face feedback. Black and white text can be harsher than intended.
Now that I understand that it's less 'wrong' and more 'not necessary' or as you put it a personal dislike I can come in off of the ledge ;-). Still there's a lot of history w/r/t the initial posting where I was encouraged to use/update Objects vs. doing what I did creating a new module and essentially renaming objs and objlists. Scaling back and considering usage of objects sent me down this current path which hasn't received as much feedback. It may not be 100% correct, but I really was hoping it wasn't wrong especially since I have working code.
Storing the UUID/Name in an @obj is the means I use to make certain decisions along the way - especially w/r/t Add/Remove logic which as I pointed out previously - there's no way to "Add" something without a key nor "Remove" it; however, the Lookup/Search logic do not need it because the hash table keeps track of the key and all the find/lookup vir*obj logic would have the @def->{uuid|name} as input for the search. The ForEach logic won't use it, although it could (more later).
Storing the keys in @obj removes a few decision points from the caller. The consumer only cares they've created an object with 1 or 2 keys in order to allow the objList code "find" those keys and make decisions based on the key rather than having multiple API's that fetch using the key name in the API name. Adding to the objlist is just that - adding and not caring/knowing how many or which tables the object is using.
While it's certainly not necessary to have UUID/Name and a LookupKeys object, it is convenient and more than just for UUID/Name. The new object is (will be) used to add bool's (active, persistent, started, etc.) that are found in multiple objects in order to have a common API to manage that. It's not required, but something possible. There are some other 'shared' type pieces of data (configFile, autostartLink, autostart, etc.) that could also make use of "sharing" data and code if
The shared code for storing a list of object is what we need and want, however adding this 'shared' data into the listing API is out of scope of that API and not all objects uses this data. Yes I can see the benefit, but IMHO it doesn't belong into that API.
so inclined. Of course the primary reason I've adjusted all those obj->def->X to be def = obj->def and def->X access is that moving the @def and @newDef inside the object was another initial goal, which needs to be rethought/reworked as that was considered too generic and possibly prone to coding errors.
For example, current add/remove API is:
==== virObjectLookupHashNew ====
virObjectLookupHashNew(virClassPtr klass, int tableElemsStart);
could be
virObjectListNew(int tableSize, bool useName, bool useUUID);
or
typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType;
virObjectListNew(int tableSize, unsigned int flags);
I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size.
Well @klass is a parameter for other virObject*New() functions, so why should virObjectLookupHashNew not have one? Or asked differently how would virObjectListNew determine it was derived from some class? Does
That's the difference, the proposed VirObjectList is a final class and currently there is no need to derive any other class from it.
that leave the hash tables in the calling vir*obj.c files or in some common file, such as vircommonobjlist.c? To me that's just a shim to virObject for each of the vir*obj.c. I don't like that solution - we
All of the code for virObjectList would be for example in virobjectlist.c|h files, it would be a separate module that uses virObject as a building block and also is tailored to contain virObject.
shouldn't create a shim that doesn't converge objects, we would take the plunge and create object code. I also don't like ObjectList if I'm using a hash table - it's not a representative name. Perhaps I'm missing something else you had in mind.
I chose the "list" because that's what the API does, it's storing a data addressed by key without any specific order, usually this type of data structure is "map" or "dictionary", so yes, the "list" is not the perfect description, we can use "map" or "dict", but "hash" is one of the possible implementation of this data structure.
@tableElemsStart is the starting size of the hash table that can grow beyond the starting size. Whether it changes names to @tableSize is just window dressing to hide the bikes in the shed. I think the reason why I used a variable was to allow different sizes for different ObjLists.
Yes, the variable should be there, you should be able to specify the size of hash table, just the name was confusing.
There may have been something else but that's long since left the short term memory. It might be for the clone or backup copy of an ObjList and wanted to have the original table size in order to create the clone. Doesn't seem I use it now though, so saving it could be dropped. Heck we could drop the initial size logic too and give virHashGrow some exercise. Although I see that has some interesting limitations w/r/t bucket chain length and larger element count tables where a max of 2048 buckets can be created allowing bucket chains to potentially grow really long. Perhaps not a problem today with test beds using 100's of objects, but IIRC there was a storage example using 1000's of volumes if not 10's of 1000's. Once you get to about 15K objs, then the probability of longer chains increases (I played around with virhashtest a bit - which currently only maxes at 250 objs, yawn).
FWIW: I went away from @useName or @useBool type logic because some tables have UUID only, some have Name only, others have both. For those with one - there is a waste of space, but if we start with table sizes of 1, it's minimized. Again, I preferred @primary and @secondary, but that idea was already squashed. In any case, here's a crudely drawn table of UUID/Name and the various vir*obj consumers.
UUID | Name ____________________|___________ NodeDev No | Yes ____________________|___________ Secret Yes | No [1] ____________________|___________ NWFilter Maybe[2]| Yes ____________________|___________ Interface No | Yes ____________________|___________ Network Yes | Yes ____________________|___________ StoragePool Yes | Yes ____________________|___________ StorageVol No | Yes ____________________|___________ Domain Yes | Yes ____________________|___________
[1] I do have patches that would make the secret usage_id be a secondary key
[2] Awful piece of history that allows the nwfilter to be defined without a UUID if saving the UUID to the file fails, thus allowing a future start to overwrite the UUID of a previous nwfilter.
There's also the possibility that Snapshots could use this, but I don't think it's as beneficial since "unpredictable ordering" for HashTables could make things AFU'd and I don't suppose you'd end up with 1000's of snapshots.
==== virObjectLookupHash(Add|Remove) ====
virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj);
virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj);
The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name.
virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid);
virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid);
or
virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid);
Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject.
Ahhh.. and this is where our fundamental difference lies. You see obj->uuid and obj->name as unnecessary and I find them useful especially for the purpose of adding/removing @obj to/from the @listObj.
Agree, for adding/removing you need all keys for all tables, having a common object with predefined keys makes it easier but also adds a limitations that you can use the list only for these objects.
It seems you'd rather see virObject* API's (or perhaps some new vircommonobjlist.c) that force the callers to know which call to use (ByName/ByUUID) and which table's were created for the object. I prefer having a common API that doesn't need to be that specific, but does the thing just using an argument.
Yes, that's exactly what I'm proposing, having a separate module, it's not even an virObject* API.
Your characterization of "flexible" is my characterization of "control". It's not like UUID/Name are going away - they're just being managed in a lower layer. My view is that the @obj as created during vir*ObjNew is what decides which tables it will need to be present in. The opposing view is I have an object, I'm going to place it in this table and possibly that table and I know where I want to put it and will manage that from the calling function entirely.
One thing I would like to see changed is *Remove returning status. Not a a lot of good happens if something during the Remove processing fails and no one is told.
==== virObjectLookupHashFind ====
virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key);
could be split into two APIs:
virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID);
Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is.
Six of one, half-dozen of the other. The consumer is still going to call one or the other ByName/ByUUID, but that leaves that decision point higher up. It's easy enough to make ByName and ByUUID API's, but they just turn into shims to the common code - so why bother?
Because a bool parameter is not extendable, imagine that we would like to add another key in future, it would force us to rewrite all the APIs.
I still prefer the concept of @primary and @secondary. Consider the table above - should it matter that a table stores UUID's? or Names? or just one or two keys? We made the decision early on during ObjNew what our keys would be. After that it's up to calling function to provide some key that we'll search on. For some consumers which use both tables, they want to Find in a specific table and that's why you'd have a boolean to say - don't search primary, search secondary.
How about a different idea. Why not make the hash table count variable (1, 2, 3, etc.) and let the caller decide how many to create and which
I had the same idea, however for some object it doesn't give us any benefit to have 4 or 5 hash tables for different keys because the number of object stored in the tables would be small. We have to think about balance between speed and memory consumption.
to use. That way the nodedev code removes the 4 or 5 FindBy type API's and replaces it with 4 or 5 hash tables to allow for lookup by key. The consumer can then have vir*ObjListFind{ByUUID|ByName}, but that ends up being a virObjectLookupHashFind(tableobj, tableidx, const char *key) where the consumer knows tableidx = 0 is UUID, tableidx = 1 is Name, etc.
==== virObjectLookupHashForEach ====
virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data);
could be
virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData);
There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do.
The @useUUID argument also applies to virObjectLookupHashSearch().
[NB: written before rereading and writing the above paragraph regarding configurable number of hash tables]
How do you choose which table? How do you know which data the caller wants? Refer to the table above - some objects have UUID and some have Name. Some consumers may even want to return a list of all objects by UUID using an input parameter and other by Name with a different input parameter. The *ForEach ends up being a multi-purpose backend for NumOf, Get{Name|UUID}, and Export functions. Typically the Get{Name|UUID} is the "primary key" for the object, but I'm just saying...
While I'm typing this response I just realized that the Get{Names|UUIDs} type API functions wouldn't need vir*obj.c specific *Callback functions to deref obj->def->{name|uuid} since obj->{uuid|name} exists! I'd still need some function to determine whether @aclfilter was passed or not for @def and I'd to know to @useUUID or not. But I could converge things even more than I have.
The usage of the virObjectList* APIs could be something like this:
virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); }
static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); }
Pavel
And here's a perfect example of an object that only lives in one table. If I call virObjectLookupHashForEach or virObjectLookupHashSearch and expect to find something in objsUUID, I'm going to be sorely disappointed. One solution is to have two API's - my solution is one API with a parameter that dictates.
That's the thing, for APIs ForEach or Search you need to use only one hash table. If there is only one, it's easy to pick the correct one, if there are two or more tables, they should contain the same objects only mapped by different keys so it doesn't matter which table is used. Pavel
Given the amount of change since this series was first posted, I have to deal with merge conflicts, the virHashSearch new argument, and using RW locks for all the objlists. So I'm going to have to repost the patches again anyway. At this time, I prefer to leave them as is for the reasons stated regarding avoiding a shim. If that's the avenue to take, then I'll let someone else go down that path. I can finish up cleanup and conversion of all the vir*obj code to use vir*Obj and vir*ObjList, but converging to a shim is something I'll avoid.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/28/2017 04:51 AM, Pavel Hrdina wrote:
On Thu, Jul 27, 2017 at 04:46:41PM -0400, John Ferlan wrote:
On 07/25/2017 05:29 AM, Pavel Hrdina wrote:
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:
Let's move the discussion [1] into correct place.
Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name.
We don't need the virObjectLookupKeys object at all.
First off - thanks for taking the time to put your thoughts here. It really helped me understand where I believe you were coming from. There are some disadvantages to working at home when it comes to sounding boards or drawing things up on a white board for those around you in an office to provide/receive face-2-face feedback. Black and white text can be harsher than intended.
Now that I understand that it's less 'wrong' and more 'not necessary' or as you put it a personal dislike I can come in off of the ledge ;-). Still there's a lot of history w/r/t the initial posting where I was encouraged to use/update Objects vs. doing what I did creating a new module and essentially renaming objs and objlists. Scaling back and considering usage of objects sent me down this current path which hasn't received as much feedback. It may not be 100% correct, but I really was hoping it wasn't wrong especially since I have working code.
Storing the UUID/Name in an @obj is the means I use to make certain decisions along the way - especially w/r/t Add/Remove logic which as I pointed out previously - there's no way to "Add" something without a key nor "Remove" it; however, the Lookup/Search logic do not need it because the hash table keeps track of the key and all the find/lookup vir*obj logic would have the @def->{uuid|name} as input for the search. The ForEach logic won't use it, although it could (more later).
Storing the keys in @obj removes a few decision points from the caller. The consumer only cares they've created an object with 1 or 2 keys in order to allow the objList code "find" those keys and make decisions based on the key rather than having multiple API's that fetch using the key name in the API name. Adding to the objlist is just that - adding and not caring/knowing how many or which tables the object is using.
While it's certainly not necessary to have UUID/Name and a LookupKeys object, it is convenient and more than just for UUID/Name. The new object is (will be) used to add bool's (active, persistent, started, etc.) that are found in multiple objects in order to have a common API to manage that. It's not required, but something possible. There are some other 'shared' type pieces of data (configFile, autostartLink, autostart, etc.) that could also make use of "sharing" data and code if
The shared code for storing a list of object is what we need and want, however adding this 'shared' data into the listing API is out of scope of that API and not all objects uses this data. Yes I can see the benefit, but IMHO it doesn't belong into that API.
And I don't believe that's the right solution. Ironically from the original series when the idea of using polymorphic or inherited objects was suggested, there seemed to be consensus that this was the correct approach. Although once implemented and shown to work, now you don't like it and would prefer some sort of common listing code. I'll stick with this, update the series and post it again with all that I've written here a bit more fresh in memory.
so inclined. Of course the primary reason I've adjusted all those obj->def->X to be def = obj->def and def->X access is that moving the @def and @newDef inside the object was another initial goal, which needs to be rethought/reworked as that was considered too generic and possibly prone to coding errors.
For example, current add/remove API is:
==== virObjectLookupHashNew ====
virObjectLookupHashNew(virClassPtr klass, int tableElemsStart);
could be
virObjectListNew(int tableSize, bool useName, bool useUUID);
or
typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType;
virObjectListNew(int tableSize, unsigned int flags);
I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size.
Well @klass is a parameter for other virObject*New() functions, so why should virObjectLookupHashNew not have one? Or asked differently how would virObjectListNew determine it was derived from some class? Does
That's the difference, the proposed VirObjectList is a final class and currently there is no need to derive any other class from it.
that leave the hash tables in the calling vir*obj.c files or in some common file, such as vircommonobjlist.c? To me that's just a shim to virObject for each of the vir*obj.c. I don't like that solution - we
All of the code for virObjectList would be for example in virobjectlist.c|h files, it would be a separate module that uses virObject as a building block and also is tailored to contain virObject.
shouldn't create a shim that doesn't converge objects, we would take the plunge and create object code. I also don't like ObjectList if I'm using a hash table - it's not a representative name. Perhaps I'm missing something else you had in mind.
I chose the "list" because that's what the API does, it's storing a data addressed by key without any specific order, usually this type of data structure is "map" or "dictionary", so yes, the "list" is not the perfect description, we can use "map" or "dict", but "hash" is one of the possible implementation of this data structure.
Lots of agita over a name... For as long as I've been doing this when someone has an API w/ List or code with @list variables that means some sort of forward or doubly-link linked list (flink and blink). Under the covers even the hash table algorithm uses a forward linked list, but it (mostly) limits the length of the list for any particular bucket to be of a particular size before the table is made to grow in order to change the population of the buckets. A map is something to show you how to get from point A to point Z with numerous route options through various points along the way that could be interesting. Although the shortest distance between any 2 points is a straight line - it's not always the fastest way. I associate the term with memory or cpu and has been a 2D/3D type concept. It allows sharing between things and changing it's size has synchronicity problems. A dictionary is something on a shelf that keeps everything alphabetically (and numerically) ordered, but is not great for searches because there's a higher likelihood of a few of the 62 letter/number buckets in the english language being overloaded while other buckets have very few entries. While "hash" isn't best name, it is the implementation just as much as the current implementation is a linked list of objects. BTW: Yes what each of the existing API's returns is a forward linked list of objects as (in the future) taken from a object containing a hash table of all objects (such as how the domain code handles things).
@tableElemsStart is the starting size of the hash table that can grow beyond the starting size. Whether it changes names to @tableSize is just window dressing to hide the bikes in the shed. I think the reason why I used a variable was to allow different sizes for different ObjLists.
Yes, the variable should be there, you should be able to specify the size of hash table, just the name was confusing.
Again name agita. The reality is - it's just the starting size. The @tableSize could change over time. In fact tableSize is even a misnomer. The libvirt hash table starting size of 10 could actually store up to 80 elements without ever changing size if it was perfectly balanced. Once there were more than 8 elements in any one of the original 10 buckets the table gets resized by a factor of 8 w/ a limitation that the next size cannot be > "8 * 2048" - taking into account the size is the number of buckets as opposed to the number of elements, but that's a different problem and discussion.
There may have been something else but that's long since left the short term memory. It might be for the clone or backup copy of an ObjList and wanted to have the original table size in order to create the clone. Doesn't seem I use it now though, so saving it could be dropped. Heck we could drop the initial size logic too and give virHashGrow some exercise. Although I see that has some interesting limitations w/r/t bucket chain length and larger element count tables where a max of 2048 buckets can be created allowing bucket chains to potentially grow really long. Perhaps not a problem today with test beds using 100's of objects, but IIRC there was a storage example using 1000's of volumes if not 10's of 1000's. Once you get to about 15K objs, then the probability of longer chains increases (I played around with virhashtest a bit - which currently only maxes at 250 objs, yawn).
FWIW: I went away from @useName or @useBool type logic because some tables have UUID only, some have Name only, others have both. For those with one - there is a waste of space, but if we start with table sizes of 1, it's minimized. Again, I preferred @primary and @secondary, but that idea was already squashed. In any case, here's a crudely drawn table of UUID/Name and the various vir*obj consumers.
UUID | Name ____________________|___________ NodeDev No | Yes ____________________|___________ Secret Yes | No [1] ____________________|___________ NWFilter Maybe[2]| Yes ____________________|___________ Interface No | Yes ____________________|___________ Network Yes | Yes ____________________|___________ StoragePool Yes | Yes ____________________|___________ StorageVol No | Yes ____________________|___________ Domain Yes | Yes ____________________|___________
[1] I do have patches that would make the secret usage_id be a secondary key
[2] Awful piece of history that allows the nwfilter to be defined without a UUID if saving the UUID to the file fails, thus allowing a future start to overwrite the UUID of a previous nwfilter.
There's also the possibility that Snapshots could use this, but I don't think it's as beneficial since "unpredictable ordering" for HashTables could make things AFU'd and I don't suppose you'd end up with 1000's of snapshots.
==== virObjectLookupHash(Add|Remove) ====
virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj);
virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj);
The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name.
virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid);
virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid);
or
virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid);
Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject.
Ahhh.. and this is where our fundamental difference lies. You see obj->uuid and obj->name as unnecessary and I find them useful especially for the purpose of adding/removing @obj to/from the @listObj.
Agree, for adding/removing you need all keys for all tables, having a common object with predefined keys makes it easier but also adds a limitations that you can use the list only for these objects.
Forcing naming by UUID or by Name is limiting to me. Does it really matter what the name of a key is to the hash object? No it just needs to be unique and for our purposes is currently a string comparison. The fact that the driver/vir*obj's "know" that it's by UUID or by Name is a higher level abstraction. Still I capitulated and altered my primary and secondary to use uuid/name and that's now causing other problems. Feels like a no win situation. When you add a 3rd key by FOO, then by having API's with By{KEY} names means you have to add a series of API's instead of modifying the object handling code to handle that. In my view, for any current driver/vir*obj that has 1 key (e.g. secrets), if I one day decide to use a second key (say by usageID), then all I do is add that key to the virSecretObjNew processing and voila under the covers the object knows how to handle things by one or two keys. The driver/vir*obj code then can use LookupByKey instead of Search for the new key. Similarly, if there were a 3rd key then a few simple modifications to add a new key to virObjectLookupKeys, modify the Add/Remove algorithm to handle the 3rd key, and teach the Find/ForEach/Search API's to handle any one of 3 keys and things should work. There's probably a couple of other missing details, but to me less work than new API's represent. The nodedev code currently has the most multiple key possibilities with ObjListFind* API's for SysfsPath, Name, WWNs, FabricWWN, Cap, and SCSIHostByWWNs. Since a key has to exist in all objs to be valid, that means for nodedev's, the only one that comes close is SysfsPath, but it doesn't work for the 'computer' object, so that's not an option. Thus (for now) nodedev is stuck using a single key. Luckily we're guaranteed some amount of uniqueness by the host/kernel algorithms.
It seems you'd rather see virObject* API's (or perhaps some new vircommonobjlist.c) that force the callers to know which call to use (ByName/ByUUID) and which table's were created for the object. I prefer having a common API that doesn't need to be that specific, but does the thing just using an argument.
Yes, that's exactly what I'm proposing, having a separate module, it's not even an virObject* API.
Your characterization of "flexible" is my characterization of "control". It's not like UUID/Name are going away - they're just being managed in a lower layer. My view is that the @obj as created during vir*ObjNew is what decides which tables it will need to be present in. The opposing view is I have an object, I'm going to place it in this table and possibly that table and I know where I want to put it and will manage that from the calling function entirely.
One thing I would like to see changed is *Remove returning status. Not a a lot of good happens if something during the Remove processing fails and no one is told.
==== virObjectLookupHashFind ====
virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key);
could be split into two APIs:
virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID);
Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is.
Six of one, half-dozen of the other. The consumer is still going to call one or the other ByName/ByUUID, but that leaves that decision point higher up. It's easy enough to make ByName and ByUUID API's, but they just turn into shims to the common code - so why bother?
Because a bool parameter is not extendable, imagine that we would like to add another key in future, it would force us to rewrite all the APIs.
So after X amount of years of using UUID/Name do you really believe there's going to be a 3rd key? Even if there were, changing the @bool to an @flags is very simple. It's not like we've never done that before and it's not like we're exposing these API's in such a way that we need to have a whole new API. It's an internal mechanism. Still what purpose does a 3rd key ever serve for an object? That probably means one didn't set unique enough primary and secondary keys. A tertiary key means searching 3 tables to make sure none of them has keys being used that the new object is trying to use as it's keyset. I sincerely doubt there's much use for that. I have a feeling we'd outright reject someone creating a 3 keys for the simple reason the API's are designed to handle at most 2 keys which should guarantee uniqueness as long as you're not forced by API naming scheme to have certain named keys.
I still prefer the concept of @primary and @secondary. Consider the table above - should it matter that a table stores UUID's? or Names? or just one or two keys? We made the decision early on during ObjNew what our keys would be. After that it's up to calling function to provide some key that we'll search on. For some consumers which use both tables, they want to Find in a specific table and that's why you'd have a boolean to say - don't search primary, search secondary.
How about a different idea. Why not make the hash table count variable (1, 2, 3, etc.) and let the caller decide how many to create and which
I had the same idea, however for some object it doesn't give us any benefit to have 4 or 5 hash tables for different keys because the number of object stored in the tables would be small. We have to think about balance between speed and memory consumption.
Right and having more than 2 keys for any object means that object *has* to have all N keys in order for creation of a tertiary table to exist and all keys have to be unique between 3 tables. I hadn't fully thought through that when originally writing. Using a variable number of hash tables is primarily beneficial for that memory consumption conundrum for those existing vir*obj that use 1 entry, IOW: Nodedev, Interface, Secret, and Volume. Of those only nodedev seems to be stuck with 1 key. Secrets could (and should) add unique_id as a Name to go along with the existing UUID. Interfaces could add MAC and Volumes could add either Key or Path. However, in order to do so because previous reviews forced me to use UUID and Name, that means I either "utilize" UUID for "MAC" or "Key" or I create a tertiary table just because I have driver/vol*obj that doesn't conform the "chosen" standard using UUID and Name as the key names. Does it at least make more sense now why I prefer non deterministic names for the table key names. I chose primary and secondary, but it could have been "foo" and "bar" too. As a side note for Interface, IIRC a virtual MAC can change that means the table key changes which makes things tricky. So doing so is "outside" my current scope.
to use. That way the nodedev code removes the 4 or 5 FindBy type API's and replaces it with 4 or 5 hash tables to allow for lookup by key. The consumer can then have vir*ObjListFind{ByUUID|ByName}, but that ends up being a virObjectLookupHashFind(tableobj, tableidx, const char *key) where the consumer knows tableidx = 0 is UUID, tableidx = 1 is Name, etc.
==== virObjectLookupHashForEach ====
virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data);
could be
virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData);
There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do.
The @useUUID argument also applies to virObjectLookupHashSearch().
[NB: written before rereading and writing the above paragraph regarding configurable number of hash tables]
How do you choose which table? How do you know which data the caller wants? Refer to the table above - some objects have UUID and some have Name. Some consumers may even want to return a list of all objects by UUID using an input parameter and other by Name with a different input parameter. The *ForEach ends up being a multi-purpose backend for NumOf, Get{Name|UUID}, and Export functions. Typically the Get{Name|UUID} is the "primary key" for the object, but I'm just saying...
While I'm typing this response I just realized that the Get{Names|UUIDs} type API functions wouldn't need vir*obj.c specific *Callback functions to deref obj->def->{name|uuid} since obj->{uuid|name} exists! I'd still need some function to determine whether @aclfilter was passed or not for @def and I'd to know to @useUUID or not. But I could converge things even more than I have.
The usage of the virObjectList* APIs could be something like this:
virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); }
static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); }
Pavel
And here's a perfect example of an object that only lives in one table. If I call virObjectLookupHashForEach or virObjectLookupHashSearch and expect to find something in objsUUID, I'm going to be sorely disappointed. One solution is to have two API's - my solution is one API with a parameter that dictates.
That's the thing, for APIs ForEach or Search you need to use only one hash table. If there is only one, it's easy to pick the correct one, if there are two or more tables, they should contain the same objects only mapped by different keys so it doesn't matter which table is used.
Pavel
OK - sure since all objects have to be in both tables *if* there are two tables, then it's a selection of one table to search. However, since my concept of @primary and @secondary was discarded - how does one know which to pick - UUID or Name? Nodedev is not going to find anything by UUID. So as another adjustment to the next posting I'll alter the names to be less deterministic. That way the Find/Search/ForEach algorithms can always don't need to know whether the upper layer is using one or two tables. John
Given the amount of change since this series was first posted, I have to deal with merge conflicts, the virHashSearch new argument, and using RW locks for all the objlists. So I'm going to have to repost the patches again anyway. At this time, I prefer to leave them as is for the reasons stated regarding avoiding a shim. If that's the avenue to take, then I'll let someone else go down that path. I can finish up cleanup and conversion of all the vir*obj code to use vir*Obj and vir*ObjList, but converging to a shim is something I'll avoid.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jul 28, 2017 at 09:44:59AM -0400, John Ferlan wrote:
On 07/28/2017 04:51 AM, Pavel Hrdina wrote:
On Thu, Jul 27, 2017 at 04:46:41PM -0400, John Ferlan wrote:
On 07/25/2017 05:29 AM, Pavel Hrdina wrote:
On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:
Let's move the discussion [1] into correct place.
Still, I must be missing something. Why is it wrong to create a new object that would have a specific use? virObjectLockable was created at some point in history and then used as a way to provide locking for a virObject that only had a @ref. Someone could still create a virObject as well and just get @refs. With the new objects based on those two objects you get a few more features that allow for add, search, lookup, remove. But you call that wrong, which is confusing to me.
What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name.
We don't need the virObjectLookupKeys object at all.
First off - thanks for taking the time to put your thoughts here. It really helped me understand where I believe you were coming from. There are some disadvantages to working at home when it comes to sounding boards or drawing things up on a white board for those around you in an office to provide/receive face-2-face feedback. Black and white text can be harsher than intended.
Now that I understand that it's less 'wrong' and more 'not necessary' or as you put it a personal dislike I can come in off of the ledge ;-). Still there's a lot of history w/r/t the initial posting where I was encouraged to use/update Objects vs. doing what I did creating a new module and essentially renaming objs and objlists. Scaling back and considering usage of objects sent me down this current path which hasn't received as much feedback. It may not be 100% correct, but I really was hoping it wasn't wrong especially since I have working code.
Storing the UUID/Name in an @obj is the means I use to make certain decisions along the way - especially w/r/t Add/Remove logic which as I pointed out previously - there's no way to "Add" something without a key nor "Remove" it; however, the Lookup/Search logic do not need it because the hash table keeps track of the key and all the find/lookup vir*obj logic would have the @def->{uuid|name} as input for the search. The ForEach logic won't use it, although it could (more later).
Storing the keys in @obj removes a few decision points from the caller. The consumer only cares they've created an object with 1 or 2 keys in order to allow the objList code "find" those keys and make decisions based on the key rather than having multiple API's that fetch using the key name in the API name. Adding to the objlist is just that - adding and not caring/knowing how many or which tables the object is using.
While it's certainly not necessary to have UUID/Name and a LookupKeys object, it is convenient and more than just for UUID/Name. The new object is (will be) used to add bool's (active, persistent, started, etc.) that are found in multiple objects in order to have a common API to manage that. It's not required, but something possible. There are some other 'shared' type pieces of data (configFile, autostartLink, autostart, etc.) that could also make use of "sharing" data and code if
The shared code for storing a list of object is what we need and want, however adding this 'shared' data into the listing API is out of scope of that API and not all objects uses this data. Yes I can see the benefit, but IMHO it doesn't belong into that API.
And I don't believe that's the right solution. Ironically from the original series when the idea of using polymorphic or inherited objects was suggested, there seemed to be consensus that this was the correct approach. Although once implemented and shown to work, now you don't like it and would prefer some sort of common listing code. I'll stick with this, update the series and post it again with all that I've written here a bit more fresh in memory.
I've replied to the original series [1] that I don't like it. [1] <https://www.redhat.com/archives/libvir-list/2017-February/msg01574.html>
so inclined. Of course the primary reason I've adjusted all those obj->def->X to be def = obj->def and def->X access is that moving the @def and @newDef inside the object was another initial goal, which needs to be rethought/reworked as that was considered too generic and possibly prone to coding errors.
For example, current add/remove API is:
==== virObjectLookupHashNew ====
virObjectLookupHashNew(virClassPtr klass, int tableElemsStart);
could be
virObjectListNew(int tableSize, bool useName, bool useUUID);
or
typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType;
virObjectListNew(int tableSize, unsigned int flags);
I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size.
Well @klass is a parameter for other virObject*New() functions, so why should virObjectLookupHashNew not have one? Or asked differently how would virObjectListNew determine it was derived from some class? Does
That's the difference, the proposed VirObjectList is a final class and currently there is no need to derive any other class from it.
that leave the hash tables in the calling vir*obj.c files or in some common file, such as vircommonobjlist.c? To me that's just a shim to virObject for each of the vir*obj.c. I don't like that solution - we
All of the code for virObjectList would be for example in virobjectlist.c|h files, it would be a separate module that uses virObject as a building block and also is tailored to contain virObject.
shouldn't create a shim that doesn't converge objects, we would take the plunge and create object code. I also don't like ObjectList if I'm using a hash table - it's not a representative name. Perhaps I'm missing something else you had in mind.
I chose the "list" because that's what the API does, it's storing a data addressed by key without any specific order, usually this type of data structure is "map" or "dictionary", so yes, the "list" is not the perfect description, we can use "map" or "dict", but "hash" is one of the possible implementation of this data structure.
Lots of agita over a name... For as long as I've been doing this when someone has an API w/ List or code with @list variables that means some sort of forward or doubly-link linked list (flink and blink). Under the covers even the hash table algorithm uses a forward linked list, but it (mostly) limits the length of the list for any particular bucket to be of a particular size before the table is made to grow in order to change the population of the buckets.
A map is something to show you how to get from point A to point Z with numerous route options through various points along the way that could be interesting. Although the shortest distance between any 2 points is a straight line - it's not always the fastest way. I associate the term with memory or cpu and has been a 2D/3D type concept. It allows sharing between things and changing it's size has synchronicity problems. A dictionary is something on a shelf that keeps everything alphabetically (and numerically) ordered, but is not great for searches because there's a higher likelihood of a few of the 62 letter/number buckets in the english language being overloaded while other buckets have very few entries. While "hash" isn't best name, it is the implementation just as much as the current implementation is a linked list of objects.
Sigh, map has a several meanings and dictionary as well. In computer science they both describe a data structure. For example python uses dictionary as a data structure of a list of pairs(key, value).
BTW: Yes what each of the existing API's returns is a forward linked list of objects as (in the future) taken from a object containing a hash table of all objects (such as how the domain code handles things).
@tableElemsStart is the starting size of the hash table that can grow beyond the starting size. Whether it changes names to @tableSize is just window dressing to hide the bikes in the shed. I think the reason why I used a variable was to allow different sizes for different ObjLists.
Yes, the variable should be there, you should be able to specify the size of hash table, just the name was confusing.
Again name agita. The reality is - it's just the starting size. The @tableSize could change over time. In fact tableSize is even a misnomer. The libvirt hash table starting size of 10 could actually store up to 80 elements without ever changing size if it was perfectly balanced. Once
I didn't know that our code modifies the size of hash table, in that case it might be pointless to specify the original size at all.
there were more than 8 elements in any one of the original 10 buckets the table gets resized by a factor of 8 w/ a limitation that the next size cannot be > "8 * 2048" - taking into account the size is the number of buckets as opposed to the number of elements, but that's a different problem and discussion.
There may have been something else but that's long since left the short term memory. It might be for the clone or backup copy of an ObjList and wanted to have the original table size in order to create the clone. Doesn't seem I use it now though, so saving it could be dropped. Heck we could drop the initial size logic too and give virHashGrow some exercise. Although I see that has some interesting limitations w/r/t bucket chain length and larger element count tables where a max of 2048 buckets can be created allowing bucket chains to potentially grow really long. Perhaps not a problem today with test beds using 100's of objects, but IIRC there was a storage example using 1000's of volumes if not 10's of 1000's. Once you get to about 15K objs, then the probability of longer chains increases (I played around with virhashtest a bit - which currently only maxes at 250 objs, yawn).
FWIW: I went away from @useName or @useBool type logic because some tables have UUID only, some have Name only, others have both. For those with one - there is a waste of space, but if we start with table sizes of 1, it's minimized. Again, I preferred @primary and @secondary, but that idea was already squashed. In any case, here's a crudely drawn table of UUID/Name and the various vir*obj consumers.
UUID | Name ____________________|___________ NodeDev No | Yes ____________________|___________ Secret Yes | No [1] ____________________|___________ NWFilter Maybe[2]| Yes ____________________|___________ Interface No | Yes ____________________|___________ Network Yes | Yes ____________________|___________ StoragePool Yes | Yes ____________________|___________ StorageVol No | Yes ____________________|___________ Domain Yes | Yes ____________________|___________
[1] I do have patches that would make the secret usage_id be a secondary key
[2] Awful piece of history that allows the nwfilter to be defined without a UUID if saving the UUID to the file fails, thus allowing a future start to overwrite the UUID of a previous nwfilter.
There's also the possibility that Snapshots could use this, but I don't think it's as beneficial since "unpredictable ordering" for HashTables could make things AFU'd and I don't suppose you'd end up with 1000's of snapshots.
==== virObjectLookupHash(Add|Remove) ====
virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj);
virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj);
The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name.
virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid);
virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid);
or
virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid);
Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject.
Ahhh.. and this is where our fundamental difference lies. You see obj->uuid and obj->name as unnecessary and I find them useful especially for the purpose of adding/removing @obj to/from the @listObj.
Agree, for adding/removing you need all keys for all tables, having a common object with predefined keys makes it easier but also adds a limitations that you can use the list only for these objects.
Forcing naming by UUID or by Name is limiting to me. Does it really matter what the name of a key is to the hash object? No it just needs to be unique and for our purposes is currently a string comparison. The fact that the driver/vir*obj's "know" that it's by UUID or by Name is a higher level abstraction. Still I capitulated and altered my primary and secondary to use uuid/name and that's now causing other problems. Feels like a no win situation.
When you add a 3rd key by FOO, then by having API's with By{KEY} names means you have to add a series of API's instead of modifying the object handling code to handle that.
In my view, for any current driver/vir*obj that has 1 key (e.g. secrets), if I one day decide to use a second key (say by usageID), then all I do is add that key to the virSecretObjNew processing and voila under the covers the object knows how to handle things by one or two keys. The driver/vir*obj code then can use LookupByKey instead of Search for the new key. Similarly, if there were a 3rd key then a few simple modifications to add a new key to virObjectLookupKeys, modify the Add/Remove algorithm to handle the 3rd key, and teach the Find/ForEach/Search API's to handle any one of 3 keys and things should work. There's probably a couple of other missing details, but to me less work than new API's represent.
The nodedev code currently has the most multiple key possibilities with ObjListFind* API's for SysfsPath, Name, WWNs, FabricWWN, Cap, and SCSIHostByWWNs. Since a key has to exist in all objs to be valid, that means for nodedev's, the only one that comes close is SysfsPath, but it doesn't work for the 'computer' object, so that's not an option. Thus (for now) nodedev is stuck using a single key. Luckily we're guaranteed some amount of uniqueness by the host/kernel algorithms.
It seems you'd rather see virObject* API's (or perhaps some new vircommonobjlist.c) that force the callers to know which call to use (ByName/ByUUID) and which table's were created for the object. I prefer having a common API that doesn't need to be that specific, but does the thing just using an argument.
Yes, that's exactly what I'm proposing, having a separate module, it's not even an virObject* API.
Your characterization of "flexible" is my characterization of "control". It's not like UUID/Name are going away - they're just being managed in a lower layer. My view is that the @obj as created during vir*ObjNew is what decides which tables it will need to be present in. The opposing view is I have an object, I'm going to place it in this table and possibly that table and I know where I want to put it and will manage that from the calling function entirely.
One thing I would like to see changed is *Remove returning status. Not a a lot of good happens if something during the Remove processing fails and no one is told.
==== virObjectLookupHashFind ====
virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key);
could be split into two APIs:
virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID);
Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is.
Six of one, half-dozen of the other. The consumer is still going to call one or the other ByName/ByUUID, but that leaves that decision point higher up. It's easy enough to make ByName and ByUUID API's, but they just turn into shims to the common code - so why bother?
Because a bool parameter is not extendable, imagine that we would like to add another key in future, it would force us to rewrite all the APIs.
So after X amount of years of using UUID/Name do you really believe there's going to be a 3rd key? Even if there were, changing the @bool to an @flags is very simple. It's not like we've never done that before and it's not like we're exposing these API's in such a way that we need to have a whole new API. It's an internal mechanism.
Still what purpose does a 3rd key ever serve for an object? That probably means one didn't set unique enough primary and secondary keys. A tertiary key means searching 3 tables to make sure none of them has keys being used that the new object is trying to use as it's keyset. I sincerely doubt there's much use for that. I have a feeling we'd outright reject someone creating a 3 keys for the simple reason the API's are designed to handle at most 2 keys which should guarantee uniqueness as long as you're not forced by API naming scheme to have certain named keys.
I still prefer the concept of @primary and @secondary. Consider the table above - should it matter that a table stores UUID's? or Names? or just one or two keys? We made the decision early on during ObjNew what our keys would be. After that it's up to calling function to provide some key that we'll search on. For some consumers which use both tables, they want to Find in a specific table and that's why you'd have a boolean to say - don't search primary, search secondary.
How about a different idea. Why not make the hash table count variable (1, 2, 3, etc.) and let the caller decide how many to create and which
I had the same idea, however for some object it doesn't give us any benefit to have 4 or 5 hash tables for different keys because the number of object stored in the tables would be small. We have to think about balance between speed and memory consumption.
Right and having more than 2 keys for any object means that object *has* to have all N keys in order for creation of a tertiary table to exist and all keys have to be unique between 3 tables. I hadn't fully thought through that when originally writing.
Using a variable number of hash tables is primarily beneficial for that memory consumption conundrum for those existing vir*obj that use 1 entry, IOW: Nodedev, Interface, Secret, and Volume.
Of those only nodedev seems to be stuck with 1 key. Secrets could (and should) add unique_id as a Name to go along with the existing UUID. Interfaces could add MAC and Volumes could add either Key or Path.
However, in order to do so because previous reviews forced me to use UUID and Name, that means I either "utilize" UUID for "MAC" or "Key" or I create a tertiary table just because I have driver/vol*obj that doesn't conform the "chosen" standard using UUID and Name as the key names.
Does it at least make more sense now why I prefer non deterministic names for the table key names. I chose primary and secondary, but it could have been "foo" and "bar" too. As a side note for Interface, IIRC a virtual MAC can change that means the table key changes which makes things tricky. So doing so is "outside" my current scope.
I'm not saying that name/uuid is perfect, I just don't like primary/secondary.
to use. That way the nodedev code removes the 4 or 5 FindBy type API's and replaces it with 4 or 5 hash tables to allow for lookup by key. The consumer can then have vir*ObjListFind{ByUUID|ByName}, but that ends up being a virObjectLookupHashFind(tableobj, tableidx, const char *key) where the consumer knows tableidx = 0 is UUID, tableidx = 1 is Name, etc.
==== virObjectLookupHashForEach ====
virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data);
could be
virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData);
There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do.
The @useUUID argument also applies to virObjectLookupHashSearch().
[NB: written before rereading and writing the above paragraph regarding configurable number of hash tables]
How do you choose which table? How do you know which data the caller wants? Refer to the table above - some objects have UUID and some have Name. Some consumers may even want to return a list of all objects by UUID using an input parameter and other by Name with a different input parameter. The *ForEach ends up being a multi-purpose backend for NumOf, Get{Name|UUID}, and Export functions. Typically the Get{Name|UUID} is the "primary key" for the object, but I'm just saying...
While I'm typing this response I just realized that the Get{Names|UUIDs} type API functions wouldn't need vir*obj.c specific *Callback functions to deref obj->def->{name|uuid} since obj->{uuid|name} exists! I'd still need some function to determine whether @aclfilter was passed or not for @def and I'd to know to @useUUID or not. But I could converge things even more than I have.
The usage of the virObjectList* APIs could be something like this:
virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); }
static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); }
Pavel
And here's a perfect example of an object that only lives in one table. If I call virObjectLookupHashForEach or virObjectLookupHashSearch and expect to find something in objsUUID, I'm going to be sorely disappointed. One solution is to have two API's - my solution is one API with a parameter that dictates.
That's the thing, for APIs ForEach or Search you need to use only one hash table. If there is only one, it's easy to pick the correct one, if there are two or more tables, they should contain the same objects only mapped by different keys so it doesn't matter which table is used.
Pavel
OK - sure since all objects have to be in both tables *if* there are two tables, then it's a selection of one table to search. However, since my concept of @primary and @secondary was discarded - how does one know which to pick - UUID or Name? Nodedev is not going to find anything by UUID. So as another adjustment to the next posting I'll alter the names to be less deterministic. That way the Find/Search/ForEach algorithms can always don't need to know whether the upper layer is using one or two tables.
It's really simple, when you create a new list for nodes you tell the virObjectListNew that you care only about name and it will internally allocate only one table for names and the second one will remain empty (NULL). In the code for Find/Search/ForEach you can check which table is NULL and which table exists and use the one that exists. Pavel
John
Given the amount of change since this series was first posted, I have to deal with merge conflicts, the virHashSearch new argument, and using RW locks for all the objlists. So I'm going to have to repost the patches again anyway. At this time, I prefer to leave them as is for the reasons stated regarding avoiding a shim. If that's the avenue to take, then I'll let someone else go down that path. I can finish up cleanup and conversion of all the vir*obj code to use vir*Obj and vir*ObjList, but converging to a shim is something I'll avoid.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Pavel Hrdina