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

[Michal Privoznik]

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 at 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