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>
---
src/conf/virstorageobj.c | 32 ++++++
src/conf/virstorageobj.h | 9 ++
src/libvirt_private.syms | 1 +
src/storage/storage_driver.c | 226 +++++++++++++++++++++++--------------------
src/test/test_driver.c | 91 ++++++++++-------
5 files changed, 218 insertions(+), 141 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 3cae34d970..eb8a57324b 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -259,6 +259,38 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
}
+/**
+ * virStoragePoolObjListSearch
+ * @pools: Pointer to pools object
+ * @search: Callback searcher helper
+ * @opaque: Opaque data to use as argument to helper
+ *
+ * Search through the @pools objects calling the @search helper using
+ * the @opaque data in order to find an object that matches some criteria
+ * and return that object locked.
+ *
+ * Returns a locked object when found and NULL when not found
+ */
+virStoragePoolObjPtr
+virStoragePoolObjListSearch(virStoragePoolObjListPtr pools,
+ virStoragePoolObjListSearcher searcher,
+ const void *opaque)
+{
+ size_t i;
+ virStoragePoolObjPtr obj;
+
+ for (i = 0; i < pools->count; i++) {
+ obj = pools->objs[i];
+ virStoragePoolObjLock(obj);
+ if (searcher(obj, opaque))
+ return obj;
+ virStoragePoolObjUnlock(obj);
+ }
+
+ return NULL;
+}
+
+
void
virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
virStoragePoolObjPtr obj)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index c84877694e..7fe4a9f68a 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -235,6 +235,15 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
virStoragePoolObjListIterator iter,
const void *opaque);
+typedef bool
+(*virStoragePoolObjListSearcher)(virStoragePoolObjPtr obj,
+ const void *opaque);
+
+virStoragePoolObjPtr
+virStoragePoolObjListSearch(virStoragePoolObjListPtr pools,
+ virStoragePoolObjListSearcher searcher,
+ const void *opaque);
+
void
virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
virStoragePoolObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 62f423649a..e4aebb3ca5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1091,6 +1091,7 @@ virStoragePoolObjIsDuplicate;
virStoragePoolObjListExport;
virStoragePoolObjListForEach;
virStoragePoolObjListFree;
+virStoragePoolObjListSearch;
virStoragePoolObjLoadAllConfigs;
virStoragePoolObjLoadAllState;
virStoragePoolObjLock;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index db327a875a..033196dc95 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1497,171 +1497,188 @@ storageVolLookupByName(virStoragePoolPtr pool,
}
+struct storageVolLookupData {
+ virConnectPtr conn;
+ const char *key;
+ char *cleanpath;
+ const char *path;
+ virStorageVolDefPtr voldef;
+};
+
+static bool
+storageVolLookupByKeyCallback(virStoragePoolObjPtr obj,
+ const void *opaque)
+{
+ struct storageVolLookupData *data = (struct storageVolLookupData *) opaque;
+
+ if (virStoragePoolObjIsActive(obj))
+ data->voldef = virStorageVolDefFindByKey(obj, data->key);
+
+ return !!data->voldef;
+}
+
+
static virStorageVolPtr
storageVolLookupByKey(virConnectPtr conn,
const char *key)
{
- size_t i;
+ virStoragePoolObjPtr obj;
+ virStoragePoolDefPtr def;
+ struct storageVolLookupData data = {
+ .conn = conn, .key = key, .voldef = NULL };
virStorageVolPtr vol = NULL;
storageDriverLock();
- for (i = 0; i < driver->pools.count && !vol; i++) {
- virStoragePoolObjPtr obj = driver->pools.objs[i];
- virStoragePoolDefPtr def;
-
- virStoragePoolObjLock(obj);
+ if ((obj = virStoragePoolObjListSearch(&driver->pools,
+ storageVolLookupByKeyCallback,
+ &data)) && data.voldef) {
def = virStoragePoolObjGetDef(obj);
- if (virStoragePoolObjIsActive(obj)) {
- virStorageVolDefPtr voldef = virStorageVolDefFindByKey(obj, key);
-
- if (voldef) {
- if (virStorageVolLookupByKeyEnsureACL(conn, def, voldef) < 0) {
- virStoragePoolObjEndAPI(&obj);
- goto cleanup;
- }
-
- vol = virGetStorageVol(conn, def->name,
- voldef->name, voldef->key,
- NULL, NULL);
- }
+ if (virStorageVolLookupByKeyEnsureACL(conn, def, data.voldef) == 0) {
+ vol = virGetStorageVol(conn, def->name,
+ data.voldef->name, data.voldef->key,
+ NULL, NULL);
}
virStoragePoolObjEndAPI(&obj);
}
+ storageDriverUnlock();
if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching key %s"), key);
- cleanup:
- storageDriverUnlock();
return vol;
}
+
+static bool
+storageVolLookupByPathCallback(virStoragePoolObjPtr obj,
+ const void *opaque)
+{
+ struct storageVolLookupData *data = (struct storageVolLookupData *) opaque;
+ virStoragePoolDefPtr def;
+ char *stable_path = NULL;
+
+ if (!virStoragePoolObjIsActive(obj))
+ return false;
+
+ def = virStoragePoolObjGetDef(obj);
+
+ switch ((virStoragePoolType) def->type) {
+ case VIR_STORAGE_POOL_DIR:
+ case VIR_STORAGE_POOL_FS:
+ case VIR_STORAGE_POOL_NETFS:
+ case VIR_STORAGE_POOL_LOGICAL:
+ case VIR_STORAGE_POOL_DISK:
+ case VIR_STORAGE_POOL_ISCSI:
+ case VIR_STORAGE_POOL_SCSI:
+ case VIR_STORAGE_POOL_MPATH:
+ case VIR_STORAGE_POOL_VSTORAGE:
+ stable_path = virStorageBackendStablePath(obj, data->cleanpath,
+ false);
+ break;
+
+ case VIR_STORAGE_POOL_GLUSTER:
+ case VIR_STORAGE_POOL_RBD:
+ case VIR_STORAGE_POOL_SHEEPDOG:
+ case VIR_STORAGE_POOL_ZFS:
+ case VIR_STORAGE_POOL_LAST:
+ ignore_value(VIR_STRDUP(stable_path, data->path));
+ break;
+ }
+
+ /* Don't break the whole lookup process if it fails on
+ * getting the stable path for some of the pools. */
+ if (!stable_path) {
+ VIR_WARN("Failed to get stable path for pool '%s'", def->name);
+ return false;
+ }
+
+ data->voldef = virStorageVolDefFindByPath(obj, stable_path);
+ VIR_FREE(stable_path);
+
+ return !!data->voldef;
+}
+
+
static virStorageVolPtr
storageVolLookupByPath(virConnectPtr conn,
const char *path)
{
- size_t i;
+ virStoragePoolObjPtr obj;
+ virStoragePoolDefPtr def;
+ struct storageVolLookupData data = {
+ .conn = conn, .path = path, .voldef = NULL };
virStorageVolPtr vol = NULL;
- char *cleanpath;
- cleanpath = virFileSanitizePath(path);
- if (!cleanpath)
+ if (!(data.cleanpath = virFileSanitizePath(path)))
return NULL;
storageDriverLock();
- for (i = 0; i < driver->pools.count && !vol; i++) {
- virStoragePoolObjPtr obj = driver->pools.objs[i];
- virStoragePoolDefPtr def;
- virStorageVolDefPtr voldef;
- char *stable_path = NULL;
-
- virStoragePoolObjLock(obj);
+ if ((obj = virStoragePoolObjListSearch(&driver->pools,
+ storageVolLookupByPathCallback,
+ &data)) && data.voldef) {
def = virStoragePoolObjGetDef(obj);
- if (!virStoragePoolObjIsActive(obj)) {
- virStoragePoolObjEndAPI(&obj);
- continue;
- }
-
- switch ((virStoragePoolType) def->type) {
- case VIR_STORAGE_POOL_DIR:
- case VIR_STORAGE_POOL_FS:
- case VIR_STORAGE_POOL_NETFS:
- case VIR_STORAGE_POOL_LOGICAL:
- case VIR_STORAGE_POOL_DISK:
- case VIR_STORAGE_POOL_ISCSI:
- case VIR_STORAGE_POOL_SCSI:
- case VIR_STORAGE_POOL_MPATH:
- case VIR_STORAGE_POOL_VSTORAGE:
- stable_path = virStorageBackendStablePath(obj,
- cleanpath,
- false);
- if (stable_path == NULL) {
- /* Don't break the whole lookup process if it fails on
- * getting the stable path for some of the pools.
- */
- VIR_WARN("Failed to get stable path for pool '%s'",
- def->name);
- virStoragePoolObjEndAPI(&obj);
- continue;
- }
- break;
-
- case VIR_STORAGE_POOL_GLUSTER:
- case VIR_STORAGE_POOL_RBD:
- case VIR_STORAGE_POOL_SHEEPDOG:
- case VIR_STORAGE_POOL_ZFS:
- case VIR_STORAGE_POOL_LAST:
- if (VIR_STRDUP(stable_path, path) < 0) {
- virStoragePoolObjEndAPI(&obj);
- goto cleanup;
- }
- break;
- }
-
- voldef = virStorageVolDefFindByPath(obj, stable_path);
- VIR_FREE(stable_path);
-
- if (voldef) {
- if (virStorageVolLookupByPathEnsureACL(conn, def, voldef) < 0) {
- virStoragePoolObjEndAPI(&obj);
- goto cleanup;
- }
-
+ if (virStorageVolLookupByPathEnsureACL(conn, def, data.voldef) == 0) {
vol = virGetStorageVol(conn, def->name,
- voldef->name, voldef->key,
+ data.voldef->name, data.voldef->key,
NULL, NULL);
}
-
virStoragePoolObjEndAPI(&obj);
}
+ storageDriverUnlock();
if (!vol) {
- if (STREQ(path, cleanpath)) {
+ if (STREQ(path, data.cleanpath)) {
virReportError(VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching path '%s'"), path);
} else {
virReportError(VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching path '%s' (%s)"),
- path, cleanpath);
+ path, data.cleanpath);
}
}
- cleanup:
- VIR_FREE(cleanpath);
- storageDriverUnlock();
+ VIR_FREE(data.cleanpath);
return vol;
}
+
+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);
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;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 72b3c6db5d..25b6592bcd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4908,6 +4908,26 @@ testStorageVolLookupByName(virStoragePoolPtr pool,
}
+struct storageVolLookupData {
+ virConnectPtr conn;
+ const char *key;
+ const char *path;
+ virStorageVolDefPtr voldef;
+};
+
+static bool
+testStorageVolLookupByKeyCallback(virStoragePoolObjPtr obj,
+ const void *opaque)
+{
+ struct storageVolLookupData *data = (struct storageVolLookupData *) opaque;
+
+ if (virStoragePoolObjIsActive(obj))
+ data->voldef = virStorageVolDefFindByKey(obj, data->key);
+
+ return !!data->voldef;
+}
+
+
static virStorageVolPtr
testStorageVolLookupByKey(virConnectPtr conn,
const char *key)
@@ -4915,34 +4935,40 @@ testStorageVolLookupByKey(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
virStoragePoolObjPtr obj;
virStoragePoolDefPtr def;
- size_t i;
- virStorageVolPtr ret = NULL;
+ struct storageVolLookupData data = {
+ .conn = conn, .key = key, .voldef = NULL };
+ virStorageVolPtr vol = NULL;
testDriverLock(privconn);
- for (i = 0; i < privconn->pools.count; i++) {
- obj = privconn->pools.objs[i];
- virStoragePoolObjLock(obj);
+ if ((obj = virStoragePoolObjListSearch(&privconn->pools,
+ testStorageVolLookupByKeyCallback,
+ &data)) && data.voldef) {
def = virStoragePoolObjGetDef(obj);
- if (virStoragePoolObjIsActive(obj)) {
- virStorageVolDefPtr privvol = virStorageVolDefFindByKey(obj, key);
-
- if (privvol) {
- ret = virGetStorageVol(conn, def->name,
- privvol->name, privvol->key,
- NULL, NULL);
- virStoragePoolObjEndAPI(&obj);
- break;
- }
- }
+ vol = virGetStorageVol(conn, def->name,
+ data.voldef->name, data.voldef->key,
+ NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
testDriverUnlock(privconn);
- if (!ret)
+ if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching key '%s'"), key);
- return ret;
+ return vol;
+}
+
+
+static bool
+testStorageVolLookupByPathCallback(virStoragePoolObjPtr obj,
+ const void *opaque)
+{
+ struct storageVolLookupData *data = (struct storageVolLookupData *) opaque;
+
+ if (virStoragePoolObjIsActive(obj))
+ data->voldef = virStorageVolDefFindByPath(obj, data->path);
+
+ return !!data->voldef;
}
@@ -4953,34 +4979,27 @@ testStorageVolLookupByPath(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
virStoragePoolObjPtr obj;
virStoragePoolDefPtr def;
- size_t i;
- virStorageVolPtr ret = NULL;
+ struct storageVolLookupData data = {
+ .conn = conn, .path = path, .voldef = NULL };
+ virStorageVolPtr vol = NULL;
testDriverLock(privconn);
- for (i = 0; i < privconn->pools.count; i++) {
- obj = privconn->pools.objs[i];
- virStoragePoolObjLock(obj);
+ if ((obj = virStoragePoolObjListSearch(&privconn->pools,
+ testStorageVolLookupByPathCallback,
+ &data)) && data.voldef) {
def = virStoragePoolObjGetDef(obj);
- if (virStoragePoolObjIsActive(obj)) {
- virStorageVolDefPtr privvol = virStorageVolDefFindByPath(obj, path);
-
- if (privvol) {
- ret = virGetStorageVol(conn, def->name,
- privvol->name, privvol->key,
- NULL, NULL);
- virStoragePoolObjEndAPI(&obj);
- break;
- }
- }
+ vol = virGetStorageVol(conn, def->name,
+ data.voldef->name, data.voldef->key,
+ NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
testDriverUnlock(privconn);
- if (!ret)
+ if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
_("no storage vol with matching path '%s'"), path);
- return ret;
+ return vol;
}
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.