[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/4] storage: Introduce virStoragePoolObjListSearch



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

...

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

>
>      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 redhat com>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]