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