On 04/06/2017 01:48 PM, John Ferlan wrote:
Mostly code motion to move storagePoolListVolumes code into virstorageobj.c
and rename to virStoragePoolObjVolumeGetNames.
Also includes a couple of variable name adjustments to keep code consistent
with other drivers.
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/conf/virstorageobj.c | 30 ++++++++++++++++++++++++++++++
src/conf/virstorageobj.h | 8 ++++++++
src/libvirt_private.syms | 1 +
src/storage/storage_driver.c | 24 ++++++------------------
src/test/test_driver.c | 21 ++++++---------------
5 files changed, 51 insertions(+), 33 deletions(-)
Again, couple of useless memset()-s here.
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index e57694c..5933618 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -214,6 +214,36 @@ virStoragePoolObjNumOfVolumes(virStorageVolDefListPtr volumes,
}
+int
+virStoragePoolObjVolumeGetNames(virStorageVolDefListPtr volumes,
+ virConnectPtr conn,
+ virStoragePoolDefPtr pooldef,
+ virStoragePoolVolumeACLFilter aclfilter,
+ char **const names,
const? This doesn't feel right. We are allocating/freeing the strings.
They are not const in any way.
+ int maxnames)
+{
+ int nnames = 0;
+ size_t i;
+
+ for (i = 0; i < volumes->count && nnames < maxnames; i++) {
+ virStorageVolDefPtr def = volumes->objs[i];
+ if (aclfilter && !aclfilter(conn, pooldef, def))
+ continue;
+ if (VIR_STRDUP(names[nnames++], def->name) < 0)
Again, this doesn't look right. Should VIR_STRDUP() fail, nnames is
increased regardless. You're probably safe now, as VIR_ALLOC() done by
caller (not direct, but an indirect one) zeroes out the memory, so
subsequent VIR_FREE() at failure label won't crash. But we should be not
relying on it. @nnames incrementation needs to be done only after
VIR_STRDUP() succeeded.
+ goto failure;
+ }
+
+ return nnames;
+
+ failure:
+ while (--nnames >= 0)
+ VIR_FREE(names[nnames]);
+ memset(names, 0, maxnames * sizeof(*names));
+
+ return -1;
+}
+
+
virStoragePoolObjPtr
virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index 3effe7a..bf88094 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -119,6 +119,14 @@ virStoragePoolObjNumOfVolumes(virStorageVolDefListPtr volumes,
virStoragePoolDefPtr pooldef,
virStoragePoolVolumeACLFilter aclfilter);
+int
+virStoragePoolObjVolumeGetNames(virStorageVolDefListPtr volumes,
+ virConnectPtr conn,
+ virStoragePoolDefPtr pooldef,
+ virStoragePoolVolumeACLFilter aclfilter,
+ char **const names,
+ int maxnames);
+
virStoragePoolObjPtr
virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9580622..a635cac 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1007,6 +1007,7 @@ virStoragePoolObjRemove;
virStoragePoolObjSaveDef;
virStoragePoolObjSourceFindDuplicate;
virStoragePoolObjUnlock;
+virStoragePoolObjVolumeGetNames;
# cpu/cpu.h
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7d2f74d..ce77fe1 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1411,14 +1411,14 @@ storagePoolNumOfVolumes(virStoragePoolPtr obj)
return ret;
}
+
static int
storagePoolListVolumes(virStoragePoolPtr obj,
char **const names,
int maxnames)
{
virStoragePoolObjPtr pool;
- size_t i;
- int n = 0;
+ int n = -1;
memset(names, 0, maxnames * sizeof(*names));
@@ -1434,24 +1434,12 @@ storagePoolListVolumes(virStoragePoolPtr obj,
goto cleanup;
}
- for (i = 0; i < pool->volumes.count && n < maxnames; i++) {
- if (!virStoragePoolListVolumesCheckACL(obj->conn, pool->def,
- pool->volumes.objs[i]))
- continue;
- if (VIR_STRDUP(names[n++], pool->volumes.objs[i]->name) < 0)
- goto cleanup;
- }
-
- virStoragePoolObjUnlock(pool);
- return n;
-
+ n = virStoragePoolObjVolumeGetNames(&pool->volumes, obj->conn, pool->def,
+ virStoragePoolListVolumesCheckACL,
+ names, maxnames);
cleanup:
virStoragePoolObjUnlock(pool);
- for (n = 0; n < maxnames; n++)
- VIR_FREE(names[n]);
-
- memset(names, 0, maxnames * sizeof(*names));
- return -1;
+ return n;
}
static int
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index a4b5833..b2840fa 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4830,6 +4830,7 @@ testStoragePoolNumOfVolumes(virStoragePoolPtr pool)
return ret;
}
+
static int
testStoragePoolListVolumes(virStoragePoolPtr pool,
char **const names,
@@ -4837,8 +4838,7 @@ testStoragePoolListVolumes(virStoragePoolPtr pool,
{
testDriverPtr privconn = pool->conn->privateData;
virStoragePoolObjPtr privpool;
- size_t i = 0;
- int n = 0;
+ int n = -1;
memset(names, 0, maxnames * sizeof(*names));
@@ -4851,24 +4851,15 @@ testStoragePoolListVolumes(virStoragePoolPtr pool,
goto cleanup;
}
- for (i = 0; i < privpool->volumes.count && n < maxnames; i++) {
- if (VIR_STRDUP(names[n++], privpool->volumes.objs[i]->name) < 0)
- goto cleanup;
- }
+ n = virStoragePoolObjVolumeGetNames(&privpool->volumes, pool->conn,
+ privpool->def, NULL, names, maxnames);
+ cleanup:
virStoragePoolObjUnlock(privpool);
return n;
This will not fly. I mean, it will as long as privpool != NULL at this
point. You need to keep the check for privpool before calling Unlock()
over it because Unlock() does not handle NULL well. Alternatively, you
can return directly if testStoragePoolObjFindByName() fails (instead of
goto cleanup).
-
- cleanup:
- for (n = 0; n < maxnames; n++)
- VIR_FREE(names[i]);
-
- memset(names, 0, maxnames * sizeof(*names));
- if (privpool)
- virStoragePoolObjUnlock(privpool);
- return -1;
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
