On 12/13/2017 11:40 AM, Michal Privoznik wrote:
On 12/05/2017 02:43 AM, John Ferlan wrote:
Now that we have a private storage pool list, we can take the next step and convert to using objects. In this case, we're going to use RWLockable objects (just like every other driver) with two hash tables for lookup by UUID or Name.
Along the way the ForEach and Search API's will be adjusted to use the related Hash API's and the various FindBy functions altered and augmented to allow for HashLookup w/ and w/o the pool lock already taken.
After virStoragePoolObjRemove we will need to virObjectUnref(obj) after to indicate the caller is "done" with it's reference. The Unlock occurs during the Remove.
The NumOf, GetNames, and Export functions all have their own callback functions to return the required data and the FindDuplicate code can use the HashSearch function callbacks.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 638 +++++++++++++++++++++++++++++-------------- src/conf/virstorageobj.h | 3 - src/libvirt_private.syms | 1 - src/storage/storage_driver.c | 8 +- src/test/test_driver.c | 7 +- 5 files changed, 449 insertions(+), 208 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index a48346b24..0e5c98bf7 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -27,6 +27,7 @@ #include "viralloc.h" #include "virerror.h" #include "virfile.h" +#include "virhash.h" #include "virlog.h" #include "virscsihost.h" #include "virstring.h" @@ -37,9 +38,12 @@ VIR_LOG_INIT("conf.virstorageobj");
static virClassPtr virStoragePoolObjClass; +static virClassPtr virStoragePoolObjListClass;
static void virStoragePoolObjDispose(void *opaque); +static void +virStoragePoolObjListDispose(void *opaque);
struct _virStorageVolDefList { @@ -63,8 +67,15 @@ struct _virStoragePoolObj { };
struct _virStoragePoolObjList { - size_t count; - virStoragePoolObjPtr *objs; + virObjectRWLockable parent; + + /* uuid string -> virStoragePoolObj mapping + * for (1), lockless lookup-by-uuid */ + virHashTable *objs; + + /* name string -> virStoragePoolObj mapping + * for (1), lockless lookup-by-name */ + virHashTable *objsName; };
@@ -77,6 +88,12 @@ virStoragePoolObjOnceInit(void) virStoragePoolObjDispose))) return -1;
+ if (!(virStoragePoolObjListClass = virClassNew(virClassForObjectRWLockable(), + "virStoragePoolObjList", + sizeof(virStoragePoolObjList), + virStoragePoolObjListDispose))) + return -1; + return 0; }
@@ -240,13 +257,15 @@ virStoragePoolObjDispose(void *opaque)
void -virStoragePoolObjListFree(virStoragePoolObjListPtr pools) +virStoragePoolObjListDispose(void *opaque) { - size_t i; - for (i = 0; i < pools->count; i++) - virObjectUnref(pools->objs[i]); - VIR_FREE(pools->objs); - VIR_FREE(pools); + virStoragePoolObjListPtr pools = opaque; + + if (!pools) + return;
This shouldn't be needed. This function is called iff pools are still not NULL.
Oh right, it's removed...
+ + virHashFree(pools->objs); + virHashFree(pools->objsName); }
@@ -255,13 +274,44 @@ virStoragePoolObjListNew(void) { virStoragePoolObjListPtr pools;
- if (VIR_ALLOC(pools) < 0) + if (virStoragePoolObjInitialize() < 0) + return NULL; + + if (!(pools = virObjectRWLockableNew(virStoragePoolObjListClass))) + return NULL; + + if (!(pools->objs = virHashCreate(20, virObjectFreeHashData)) || + !(pools->objsName = virHashCreate(20, virObjectFreeHashData))) { + virObjectUnref(pools); return NULL; + }
return pools; }
+struct _virStoragePoolObjListForEachData { + virStoragePoolObjListIterator iter; + const void *opaque; +}; + +static int +virStoragePoolObjListForEachCb(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virStoragePoolObjPtr obj = payload; + struct _virStoragePoolObjListForEachData *data = + (struct _virStoragePoolObjListForEachData *)opaque;
Do we need this typecast? I don't think so. You're assigning void *payload to virStoragePoolObjPtr obj directly, without any typecast.
True - it's probably a holdover from undoing something. Been too long to remember though. Consider it gone.
+ + virObjectLock(obj); + data->iter(obj, data->opaque); + virObjectUnlock(obj); + + return 0; +} + + /** * virStoragePoolObjListForEach * @pools: Pointer to pools object @@ -279,15 +329,35 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools, virStoragePoolObjListIterator iter, const void *opaque) { - size_t i; - virStoragePoolObjPtr obj; + struct _virStoragePoolObjListForEachData data = { .iter = iter, + .opaque = opaque };
- for (i = 0; i < pools->count; i++) { - obj = pools->objs[i]; - virObjectLock(obj); - iter(obj, opaque); - virObjectUnlock(obj); - } + virObjectRWLockRead(pools); + virHashForEach(pools->objs, virStoragePoolObjListForEachCb, &data); + virObjectRWUnlock(pools); +} + + +struct _virStoragePoolObjListSearchData { + virStoragePoolObjListSearcher searcher; + const void *opaque; +}; + + +static int +virStoragePoolObjListSearchCb(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virStoragePoolObjPtr obj = (virStoragePoolObjPtr) payload; + struct _virStoragePoolObjListSearchData *data = + (struct _virStoragePoolObjListSearchData *)opaque;
Of course, typecast is needed here because we need to drop 'const'. Grrr. I wonder if locking an object is considered as modifying it. IOW if virObjectLock() should take 'void *' or 'const void *'.
I would think taking a lock is considered violation of const-ness. I can certainly see cause for why a Search callback would say don't change something though. Taking a different approach - if the caller knew that the opaque was a LockableObject, then it could do the right thing and keep the const argument. It could probably even be better if it was a RWReadLock. But that's a different rabbit-hole that I'm not sure I want to jump into yet. Although I did dip my toes in that water and got more or less rejected, so this is as close as I think I'm going to get for now. Thanks for the review - John
+ + virObjectLock(obj); + if (data->searcher(obj, data->opaque)) + return 1; + virObjectUnlock(obj); + return 0; }
Michal