Re: [libvirt] [PATCH 3/4] storage: Introduce virStoragePoolObjListSearch
On 11/23/2017 09:00 AM, Erik Skultety wrote:
On Thu, Nov 16, 2017 at 11:58:04AM -0500, John Ferlan wrote:
> Create an API to search through the storage pool objects looking for
> a specific truism from a callback API in order to return the specific
> storage pool object that is desired.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
As for the searcher that is a copy-paste of the 'for each iterator' with a
single difference, I assume the list is going to be converted to a hash table
further on, right?
...
Your assumption is correct... And the fact that Hash is used instead of
linear search would be obfuscated by the virStoragePoolObjList API's.
IIRC this driver ended up being a little different than others w/r/t
what's done that requires keeping certain functionality in the driver as
opposed to moving it to virstoragepoolobj.c. Of course it's been a while
since I first thought about it though! This adaptation comes from the
original pile I wrote in Jan/Feb...
>
> +
> +static bool
> +storagePoolLookupByTargetPathCallback(virStoragePoolObjPtr obj,
> + const void *opaque)
> +{
> + const char *path = opaque;
> + virStoragePoolDefPtr def;
> +
> + if (!virStoragePoolObjIsActive(obj))
> + return false;
> +
> + def = virStoragePoolObjGetDef(obj);
> + return STREQ(path, def->target.path);
> +}
> +
> +
> virStoragePoolPtr
> storagePoolLookupByTargetPath(virConnectPtr conn,
> const char *path)
> {
> - size_t i;
> + virStoragePoolObjPtr obj;
> + virStoragePoolDefPtr def;
> virStoragePoolPtr pool = NULL;
> char *cleanpath;
>
> cleanpath = virFileSanitizePath(path);
> if (!cleanpath)
> return NULL;
> + VIR_FREE(cleanpath);
Existing prior your patch, but I'm clearly missing the point of running
virFileSanitizePath here, since it can only return NULL on a failed allocation
and the way we're treating it now is "it either failed or some sanitization
may or may not have happened" - my question therefore is, why don't we care
about the result of sanitizing the path but we have to run it anyway?
Hmm.. no idea... Good question... It was added by commit id
'5ab746b83'... I just saw that it wasn't used after getting it so I
moved the VIR_FREE.
I wonder if the original intent was to copy the same error message logic
that storageVolLookupByPath uses... Or even use it in the STREQ
comparison. I'll mark this as something to look at prior to the next
series...
tks -
John
>
> storageDriverLock();
> - for (i = 0; i < driver->pools.count && !pool; i++) {
> - virStoragePoolObjPtr obj = driver->pools.objs[i];
> - virStoragePoolDefPtr def;
> -
> - virStoragePoolObjLock(obj);
> + if ((obj == virStoragePoolObjListSearch(&driver->pools,
> + storagePoolLookupByTargetPathCallback,
> + path))) {
> def = virStoragePoolObjGetDef(obj);
> -
> - if (!virStoragePoolObjIsActive(obj)) {
> - virStoragePoolObjEndAPI(&obj);
> - continue;
> - }
> -
> - if (STREQ(path, def->target.path))
> - pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
> -
> + pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
> virStoragePoolObjEndAPI(&obj);
> }
> storageDriverUnlock();
> @@ -1672,7 +1689,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn,
> path);
> }
>
> - VIR_FREE(cleanpath);
> return pool;
> }
>
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>