Re: [libvirt] [PATCH 3/4] storage: Convert virStoragePoolObjList to use virObjectRWLockable

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(a)redhat.com&gt; > --- > 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.

> + > + 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.

> + > + 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 *'.

> + > + virObjectLock(obj); > + if (data->searcher(obj, data->opaque)) > + return 1; > + virObjectUnlock(obj); > + return 0; > } Michal