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 <jfer...@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.
> +
> + 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
