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

23 Nov 2017


      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>

    