[libvirt] [PATCH 0/7] storage: Create common virStorageObj* APIs
Merge code in storage_driver.c and test_driver into virstorageobj to count the number of volumes/pools, to return a list of names, return the collected list of objects for volumes/pools. For volumes, that's moved code, while for pools that's just changing the export API to take an address of 'devobjs' to follow other usages. I also added code to converge the FindBy{UUID|Name} FWIW: This is part of the common driver objects code I've been working through. I figured I will post each driver separately rather than one John Ferlan (7): storage: Introduce virStoragePoolObjNumOfVolumes storage: Introduce virStoragePoolObjVolumeGetNames storage: Introduce virStoragePoolObjVolumeListExport storage: Introduce virStoragePoolObjNumOfStoragePools storage: Introduce virStoragePoolObjGetNames storage: Pass driver arg by ref storage: Create helpers to perform FindByUUID and FindByName src/conf/virstorageobj.c | 165 +++++++++++++++++++++++- src/conf/virstorageobj.h | 45 ++++++- src/libvirt_private.syms | 5 + src/storage/storage_driver.c | 297 +++++++++++++------------------------------ src/test/test_driver.c | 131 +++++-------------- 5 files changed, 332 insertions(+), 311 deletions(-) -- 2.9.3
Unify the NumOfVolumes API into virstorageobj.c from storage_driver and test_driver. The only real difference between the two is the test driver doesn't call using the aclfilter API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 19 +++++++++++++++++++ src/conf/virstorageobj.h | 11 +++++++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 10 +++------- src/test/test_driver.c | 3 ++- 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 34f2eb7..e57694c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -195,6 +195,25 @@ virStorageVolDefFindByName(virStoragePoolObjPtr pool, } +int +virStoragePoolObjNumOfVolumes(virStorageVolDefListPtr volumes, + virConnectPtr conn, + virStoragePoolDefPtr pooldef, + virStoragePoolVolumeACLFilter aclfilter) +{ + int nvolumes = 0; + size_t i; + + for (i = 0; i < volumes->count; i++) { + virStorageVolDefPtr def = volumes->objs[i]; + if (aclfilter && aclfilter(conn, pooldef, def)) + nvolumes++; + } + + return nvolumes; +} + + virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index ecba94e..3effe7a 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -108,6 +108,17 @@ virStorageVolDefFindByName(virStoragePoolObjPtr pool, void virStoragePoolObjClearVols(virStoragePoolObjPtr pool); +typedef bool +(*virStoragePoolVolumeACLFilter)(virConnectPtr conn, + virStoragePoolDefPtr pool, + virStorageVolDefPtr def); + +int +virStoragePoolObjNumOfVolumes(virStorageVolDefListPtr volumes, + virConnectPtr conn, + virStoragePoolDefPtr pooldef, + virStoragePoolVolumeACLFilter aclfilter); + virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 92083e5..9580622 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1002,6 +1002,7 @@ virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; virStoragePoolObjLock; +virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjSourceFindDuplicate; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index fea7698..7d2f74d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1390,7 +1390,6 @@ storagePoolNumOfVolumes(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; int ret = -1; - size_t i; if (!(pool = virStoragePoolObjFromStoragePool(obj))) return -1; @@ -1403,12 +1402,9 @@ storagePoolNumOfVolumes(virStoragePoolPtr obj) _("storage pool '%s' is not active"), pool->def->name); goto cleanup; } - ret = 0; - for (i = 0; i < pool->volumes.count; i++) { - if (virStoragePoolNumOfVolumesCheckACL(obj->conn, pool->def, - pool->volumes.objs[i])) - ret++; - } + + ret = virStoragePoolObjNumOfVolumes(&pool->volumes, obj->conn, pool->def, + virStoragePoolNumOfVolumesCheckACL); cleanup: virStoragePoolObjUnlock(pool); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index cce4d2d..a4b5833 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4821,7 +4821,8 @@ testStoragePoolNumOfVolumes(virStoragePoolPtr pool) goto cleanup; } - ret = privpool->volumes.count; + ret = virStoragePoolObjNumOfVolumes(&privpool->volumes, pool->conn, + privpool->def, NULL); cleanup: if (privpool) -- 2.9.3
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@redhat.com> --- 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(-) 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, + 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) + 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; - - cleanup: - for (n = 0; n < maxnames; n++) - VIR_FREE(names[i]); - - memset(names, 0, maxnames * sizeof(*names)); - if (privpool) - virStoragePoolObjUnlock(privpool); - return -1; } + static int testStoragePoolListAllVolumes(virStoragePoolPtr obj, virStorageVolPtr **vols, -- 2.9.3
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@redhat.com> --- 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
On 04/10/2017 07:52 AM, Michal Privoznik wrote:
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@redhat.com> --- 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.
"char **const names" is how this has been defined since before I touched it! It'd need to be something changed in many more places.
+ 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.
True, but it does follow from whence it came w/ variable name changes. I will change it though. Guess this also trivially ;-) needs to be adjusted in the (now pushed, <sigh>) nodedev changes as well...
+ 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).
Oh right - good catch. I'll return -1 if the ObjFindByName fails instead (similar to how the storage_driver code does things) Tks John
- - cleanup: - for (n = 0; n < maxnames; n++) - VIR_FREE(names[i]); - - memset(names, 0, maxnames * sizeof(*names)); - if (privpool) - virStoragePoolObjUnlock(privpool); - return -1;
Michal
Essentially code motion to move the storage/test driver ListAllVolumes logic into virstorageobj.c Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/virstorageobj.h | 7 +++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 37 ++++------------------------------ src/test/test_driver.c | 40 ++++--------------------------------- 5 files changed, 63 insertions(+), 69 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 5933618..2484517 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -244,6 +244,53 @@ virStoragePoolObjVolumeGetNames(virStorageVolDefListPtr volumes, } +int +virStoragePoolObjVolumeListExport(virConnectPtr conn, + virStorageVolDefListPtr volumes, + virStoragePoolDefPtr pooldef, + virStorageVolPtr **vols, + virStoragePoolVolumeACLFilter aclfilter) +{ + int ret = -1; + size_t i; + virStorageVolPtr *tmp_vols = NULL; + virStorageVolPtr vol = NULL; + int nvols = 0; + + /* Just returns the volumes count */ + if (!vols) { + ret = volumes->count; + goto cleanup; + } + + if (VIR_ALLOC_N(tmp_vols, volumes->count + 1) < 0) + goto cleanup; + + for (i = 0; i < volumes->count; i++) { + virStorageVolDefPtr def = volumes->objs[i]; + if (aclfilter && !aclfilter(conn, pooldef, def)) + continue; + if (!(vol = virGetStorageVol(conn, pooldef->name, def->name, def->key, + NULL, NULL))) + goto cleanup; + tmp_vols[nvols++] = vol; + } + + *vols = tmp_vols; + tmp_vols = NULL; + ret = nvols; + + cleanup: + if (tmp_vols) { + for (i = 0; i < nvols; i++) + virObjectUnref(tmp_vols[i]); + VIR_FREE(tmp_vols); + } + + return ret; +} + + virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def) diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index bf88094..72daa5a 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -127,6 +127,13 @@ virStoragePoolObjVolumeGetNames(virStorageVolDefListPtr volumes, char **const names, int maxnames); +int +virStoragePoolObjVolumeListExport(virConnectPtr conn, + virStorageVolDefListPtr volumes, + virStoragePoolDefPtr pooldef, + virStorageVolPtr **vols, + virStoragePoolVolumeACLFilter aclfilter); + virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a635cac..6298019 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1008,6 +1008,7 @@ virStoragePoolObjSaveDef; virStoragePoolObjSourceFindDuplicate; virStoragePoolObjUnlock; virStoragePoolObjVolumeGetNames; +virStoragePoolObjVolumeListExport; # cpu/cpu.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ce77fe1..475e332 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1442,16 +1442,13 @@ storagePoolListVolumes(virStoragePoolPtr obj, return n; } + static int storagePoolListAllVolumes(virStoragePoolPtr pool, virStorageVolPtr **vols, unsigned int flags) { virStoragePoolObjPtr obj; - size_t i; - virStorageVolPtr *tmp_vols = NULL; - virStorageVolPtr vol = NULL; - int nvols = 0; int ret = -1; virCheckFlags(0, -1); @@ -1468,38 +1465,12 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, goto cleanup; } - /* Just returns the volumes count */ - if (!vols) { - ret = obj->volumes.count; - goto cleanup; - } + ret = virStoragePoolObjVolumeListExport(pool->conn, &obj->volumes, + obj->def, vols, + virStoragePoolListAllVolumesCheckACL); - if (VIR_ALLOC_N(tmp_vols, obj->volumes.count + 1) < 0) - goto cleanup; - - for (i = 0; i < obj->volumes.count; i++) { - if (!virStoragePoolListAllVolumesCheckACL(pool->conn, obj->def, - obj->volumes.objs[i])) - continue; - if (!(vol = virGetStorageVol(pool->conn, obj->def->name, - obj->volumes.objs[i]->name, - obj->volumes.objs[i]->key, - NULL, NULL))) - goto cleanup; - tmp_vols[nvols++] = vol; - } - - *vols = tmp_vols; - tmp_vols = NULL; - ret = nvols; cleanup: - if (tmp_vols) { - for (i = 0; i < nvols; i++) - virObjectUnref(tmp_vols[i]); - VIR_FREE(tmp_vols); - } - virStoragePoolObjUnlock(obj); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b2840fa..44bd47c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4867,16 +4867,12 @@ testStoragePoolListAllVolumes(virStoragePoolPtr obj, { testDriverPtr privconn = obj->conn->privateData; virStoragePoolObjPtr pool; - size_t i; - virStorageVolPtr *tmp_vols = NULL; - virStorageVolPtr vol = NULL; - int nvols = 0; int ret = -1; virCheckFlags(0, -1); if (!(pool = testStoragePoolObjFindByUUID(privconn, obj->uuid))) - goto cleanup; + return -1; if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4884,39 +4880,11 @@ testStoragePoolListAllVolumes(virStoragePoolPtr obj, goto cleanup; } - /* Just returns the volumes count */ - if (!vols) { - ret = pool->volumes.count; - goto cleanup; - } - - if (VIR_ALLOC_N(tmp_vols, pool->volumes.count + 1) < 0) - goto cleanup; - - for (i = 0; i < pool->volumes.count; i++) { - if (!(vol = virGetStorageVol(obj->conn, pool->def->name, - pool->volumes.objs[i]->name, - pool->volumes.objs[i]->key, - NULL, NULL))) - goto cleanup; - tmp_vols[nvols++] = vol; - } - - *vols = tmp_vols; - tmp_vols = NULL; - ret = nvols; + ret = virStoragePoolObjVolumeListExport(obj->conn, &pool->volumes, + pool->def, vols, NULL); cleanup: - if (tmp_vols) { - for (i = 0; i < nvols; i++) { - if (tmp_vols[i]) - virStorageVolFree(tmp_vols[i]); - } - VIR_FREE(tmp_vols); - } - - if (pool) - virStoragePoolObjUnlock(pool); + virStoragePoolObjUnlock(pool); return ret; } -- 2.9.3
Unify the NumOf[Defined]StoragePools API into virstorageobj.c from storage_driver and test_driver. The only real difference between the two is the test driver doesn't call using the aclfilter API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 24 ++++++++++++++++++++++++ src/conf/virstorageobj.h | 9 +++++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 22 ++++------------------ src/test/test_driver.c | 17 ++++++----------- 5 files changed, 44 insertions(+), 29 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2484517..f0201aa 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -556,6 +556,30 @@ virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool) } +int +virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter) +{ + int npools = 0; + size_t i; + + for (i = 0; i < pools->count; i++) { + virStoragePoolObjPtr obj = pools->objs[i]; + virStoragePoolObjLock(obj); + if (!aclfilter || aclfilter(conn, obj->def)) { + if ((wantActive && virStoragePoolObjIsActive(obj)) || + (!wantActive && !virStoragePoolObjIsActive(obj))) + npools++; + } + virStoragePoolObjUnlock(obj); + } + + return npools; +} + + /* * virStoragePoolObjIsDuplicate: * @doms : virStoragePoolObjListPtr to search diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 72daa5a..5eeda1e 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -146,6 +146,15 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, int virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool); +typedef bool (*virStoragePoolObjListACLFilter)(virConnectPtr conn, + virStoragePoolDefPtr def); + +int +virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter); + void virStoragePoolObjFree(virStoragePoolObjPtr pool); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6298019..7bc3bc4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1002,6 +1002,7 @@ virStoragePoolObjListFree; virStoragePoolObjLoadAllConfigs; virStoragePoolObjLoadAllState; virStoragePoolObjLock; +virStoragePoolObjNumOfStoragePools; virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; virStoragePoolObjSaveDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 475e332..065380b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -477,21 +477,14 @@ storagePoolLookupByVolume(virStorageVolPtr vol) static int storageConnectNumOfStoragePools(virConnectPtr conn) { - size_t i; int nactive = 0; if (virConnectNumOfStoragePoolsEnsureACL(conn) < 0) return -1; storageDriverLock(); - for (i = 0; i < driver->pools.count; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectNumOfStoragePoolsCheckACL(conn, obj->def) && - virStoragePoolObjIsActive(obj)) - nactive++; - virStoragePoolObjUnlock(obj); - } + nactive = virStoragePoolObjNumOfStoragePools(&driver->pools, conn, true, + virConnectNumOfStoragePoolsCheckACL); storageDriverUnlock(); return nactive; @@ -536,21 +529,14 @@ storageConnectListStoragePools(virConnectPtr conn, static int storageConnectNumOfDefinedStoragePools(virConnectPtr conn) { - size_t i; int nactive = 0; if (virConnectNumOfDefinedStoragePoolsEnsureACL(conn) < 0) return -1; storageDriverLock(); - for (i = 0; i < driver->pools.count; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectNumOfDefinedStoragePoolsCheckACL(conn, obj->def) && - !virStoragePoolObjIsActive(obj)) - nactive++; - virStoragePoolObjUnlock(obj); - } + nactive = virStoragePoolObjNumOfStoragePools(&driver->pools, conn, false, + virConnectNumOfDefinedStoragePoolsCheckACL); storageDriverUnlock(); return nactive; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 44bd47c..4fb9915 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4115,17 +4115,16 @@ testStoragePoolLookupByVolume(virStorageVolPtr vol) return testStoragePoolLookupByName(vol->conn, vol->pool); } + static int testConnectNumOfStoragePools(virConnectPtr conn) { testDriverPtr privconn = conn->privateData; int numActive = 0; - size_t i; testDriverLock(privconn); - for (i = 0; i < privconn->pools.count; i++) - if (virStoragePoolObjIsActive(privconn->pools.objs[i])) - numActive++; + numActive = virStoragePoolObjNumOfStoragePools(&privconn->pools, conn, + true, NULL); testDriverUnlock(privconn); return numActive; @@ -4162,20 +4161,16 @@ testConnectListStoragePools(virConnectPtr conn, return -1; } + static int testConnectNumOfDefinedStoragePools(virConnectPtr conn) { testDriverPtr privconn = conn->privateData; int numInactive = 0; - size_t i; testDriverLock(privconn); - for (i = 0; i < privconn->pools.count; i++) { - virStoragePoolObjLock(privconn->pools.objs[i]); - if (!virStoragePoolObjIsActive(privconn->pools.objs[i])) - numInactive++; - virStoragePoolObjUnlock(privconn->pools.objs[i]); - } + numInactive = virStoragePoolObjNumOfStoragePools(&privconn->pools, conn, + false, NULL); testDriverUnlock(privconn); return numInactive; -- 2.9.3
On 04/06/2017 01:48 PM, John Ferlan wrote:
Unify the NumOf[Defined]StoragePools API into virstorageobj.c from storage_driver and test_driver. The only real difference between the two is the test driver doesn't call using the aclfilter API.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 24 ++++++++++++++++++++++++ src/conf/virstorageobj.h | 9 +++++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 22 ++++------------------ src/test/test_driver.c | 17 ++++++----------- 5 files changed, 44 insertions(+), 29 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2484517..f0201aa 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -556,6 +556,30 @@ virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool) }
+int +virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter) +{ + int npools = 0; + size_t i; + + for (i = 0; i < pools->count; i++) { + virStoragePoolObjPtr obj = pools->objs[i]; + virStoragePoolObjLock(obj); + if (!aclfilter || aclfilter(conn, obj->def)) { + if ((wantActive && virStoragePoolObjIsActive(obj)) || + (!wantActive && !virStoragePoolObjIsActive(obj)))
Again, if (wantActive == virStoragePoolObjIsActive()) ...
+ npools++; + } + virStoragePoolObjUnlock(obj); + } + + return npools; +} + +
Michal
Mostly code motion to move storageConnectList[Defined]StoragePools and similar test driver code into virstorageobj.c and rename to virStoragePoolObjGetNames. Also includes a couple of variable name adjustments to keep code consistent with other drivers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 37 ++++++++++++++++++++++++++++ src/conf/virstorageobj.h | 8 ++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 58 +++++++++++--------------------------------- src/test/test_driver.c | 48 +++++++++--------------------------- 5 files changed, 72 insertions(+), 80 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index f0201aa..a9367a2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -580,6 +580,43 @@ virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, } +int +virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter, + char **const names, + int maxnames) +{ + int nnames = 0; + size_t i; + + for (i = 0; i < pools->count && nnames < maxnames; i++) { + virStoragePoolObjPtr obj = pools->objs[i]; + virStoragePoolObjLock(obj); + if (!aclfilter || aclfilter(conn, obj->def)) { + if ((wantActive && virStoragePoolObjIsActive(obj)) || + (!wantActive && !virStoragePoolObjIsActive(obj))) { + if (VIR_STRDUP(names[nnames++], obj->def->name) < 0) { + virStoragePoolObjUnlock(obj); + goto failure; + } + } + } + virStoragePoolObjUnlock(obj); + } + + return nnames; + + failure: + while (--nnames >= 0) + VIR_FREE(names[nnames]); + memset(names, 0, maxnames * sizeof(*names)); + + return -1; +} + + /* * virStoragePoolObjIsDuplicate: * @doms : virStoragePoolObjListPtr to search diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 5eeda1e..b651865 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -155,6 +155,14 @@ virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, bool wantActive, virStoragePoolObjListACLFilter aclfilter); +int +virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter, + char **const names, + int maxnames); + void virStoragePoolObjFree(virStoragePoolObjPtr pool); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7bc3bc4..233110a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -996,6 +996,7 @@ virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; +virStoragePoolObjGetNames; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; virStoragePoolObjListFree; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 065380b..9493da8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -490,40 +490,25 @@ storageConnectNumOfStoragePools(virConnectPtr conn) return nactive; } + static int storageConnectListStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { int got = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names)); if (virConnectListStoragePoolsEnsureACL(conn) < 0) return -1; storageDriverLock(); - for (i = 0; i < driver->pools.count && got < nnames; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectListStoragePoolsCheckACL(conn, obj->def) && - virStoragePoolObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virStoragePoolObjUnlock(obj); - goto cleanup; - } - got++; - } - virStoragePoolObjUnlock(obj); - } + got = virStoragePoolObjGetNames(&driver->pools, conn, true, + virConnectListStoragePoolsCheckACL, + names, maxnames); storageDriverUnlock(); return got; - - cleanup: - storageDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - memset(names, 0, nnames * sizeof(*names)); - return -1; } static int @@ -542,40 +527,25 @@ storageConnectNumOfDefinedStoragePools(virConnectPtr conn) return nactive; } + static int storageConnectListDefinedStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { int got = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names)); if (virConnectListDefinedStoragePoolsEnsureACL(conn) < 0) return -1; storageDriverLock(); - for (i = 0; i < driver->pools.count && got < nnames; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectListDefinedStoragePoolsCheckACL(conn, obj->def) && - !virStoragePoolObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virStoragePoolObjUnlock(obj); - goto cleanup; - } - got++; - } - virStoragePoolObjUnlock(obj); - } + got = virStoragePoolObjGetNames(&driver->pools, conn, false, + virConnectListDefinedStoragePoolsCheckACL, + names, maxnames); storageDriverUnlock(); return got; - - cleanup: - storageDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - memset(names, 0, nnames * sizeof(*names)); - return -1; } /* This method is required to be re-entrant / thread safe, so diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4fb9915..1a6601e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4130,35 +4130,23 @@ testConnectNumOfStoragePools(virConnectPtr conn) return numActive; } + static int testConnectListStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { testDriverPtr privconn = conn->privateData; int n = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names)); testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->pools.count && n < nnames; i++) { - virStoragePoolObjLock(privconn->pools.objs[i]); - if (virStoragePoolObjIsActive(privconn->pools.objs[i]) && - VIR_STRDUP(names[n++], privconn->pools.objs[i]->def->name) < 0) { - virStoragePoolObjUnlock(privconn->pools.objs[i]); - goto error; - } - virStoragePoolObjUnlock(privconn->pools.objs[i]); - } + n = virStoragePoolObjGetNames(&privconn->pools, conn, true, NULL, + names, maxnames); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } @@ -4176,35 +4164,23 @@ testConnectNumOfDefinedStoragePools(virConnectPtr conn) return numInactive; } + static int testConnectListDefinedStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { testDriverPtr privconn = conn->privateData; int n = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names)); testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->pools.count && n < nnames; i++) { - virStoragePoolObjLock(privconn->pools.objs[i]); - if (!virStoragePoolObjIsActive(privconn->pools.objs[i]) && - VIR_STRDUP(names[n++], privconn->pools.objs[i]->def->name) < 0) { - virStoragePoolObjUnlock(privconn->pools.objs[i]); - goto error; - } - virStoragePoolObjUnlock(privconn->pools.objs[i]); - } + n = virStoragePoolObjGetNames(&privconn->pools, conn, false, NULL, + names, maxnames); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static int -- 2.9.3
On 04/06/2017 01:48 PM, John Ferlan wrote:
Mostly code motion to move storageConnectList[Defined]StoragePools and similar test driver code into virstorageobj.c and rename to virStoragePoolObjGetNames.
Also includes a couple of variable name adjustments to keep code consistent with other drivers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 37 ++++++++++++++++++++++++++++ src/conf/virstorageobj.h | 8 ++++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 58 +++++++++++--------------------------------- src/test/test_driver.c | 48 +++++++++--------------------------- 5 files changed, 72 insertions(+), 80 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index f0201aa..a9367a2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -580,6 +580,43 @@ virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, }
+int +virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter, + char **const names,
const?
+ int maxnames) +{ + int nnames = 0; + size_t i; + + for (i = 0; i < pools->count && nnames < maxnames; i++) { + virStoragePoolObjPtr obj = pools->objs[i]; + virStoragePoolObjLock(obj); + if (!aclfilter || aclfilter(conn, obj->def)) { + if ((wantActive && virStoragePoolObjIsActive(obj)) || + (!wantActive && !virStoragePoolObjIsActive(obj))) {
;-)
+ if (VIR_STRDUP(names[nnames++], obj->def->name) < 0) {
Again, put nnames++ after the if() block.
+ virStoragePoolObjUnlock(obj); + goto failure; + } + } + } + virStoragePoolObjUnlock(obj); + } + + return nnames; + + failure: + while (--nnames >= 0) + VIR_FREE(names[nnames]); + memset(names, 0, maxnames * sizeof(*names));
;-)
+ + return -1; +} + + /* * virStoragePoolObjIsDuplicate: * @doms : virStoragePoolObjListPtr to search diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 5eeda1e..b651865 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -155,6 +155,14 @@ virStoragePoolObjNumOfStoragePools(virStoragePoolObjListPtr pools, bool wantActive, virStoragePoolObjListACLFilter aclfilter);
+int +virStoragePoolObjGetNames(virStoragePoolObjListPtr pools, + virConnectPtr conn, + bool wantActive, + virStoragePoolObjListACLFilter aclfilter, + char **const names, + int maxnames); + void virStoragePoolObjFree(virStoragePoolObjPtr pool);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7bc3bc4..233110a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -996,6 +996,7 @@ virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; +virStoragePoolObjGetNames; virStoragePoolObjIsDuplicate; virStoragePoolObjListExport; virStoragePoolObjListFree; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 065380b..9493da8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -490,40 +490,25 @@ storageConnectNumOfStoragePools(virConnectPtr conn) return nactive; }
+ static int storageConnectListStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { int got = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names));
;-)
if (virConnectListStoragePoolsEnsureACL(conn) < 0) return -1;
storageDriverLock(); - for (i = 0; i < driver->pools.count && got < nnames; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectListStoragePoolsCheckACL(conn, obj->def) && - virStoragePoolObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virStoragePoolObjUnlock(obj); - goto cleanup; - } - got++; - } - virStoragePoolObjUnlock(obj); - } + got = virStoragePoolObjGetNames(&driver->pools, conn, true, + virConnectListStoragePoolsCheckACL, + names, maxnames); storageDriverUnlock(); return got; - - cleanup: - storageDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - memset(names, 0, nnames * sizeof(*names)); - return -1; }
static int @@ -542,40 +527,25 @@ storageConnectNumOfDefinedStoragePools(virConnectPtr conn) return nactive; }
+ static int storageConnectListDefinedStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { int got = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names));
;-)
if (virConnectListDefinedStoragePoolsEnsureACL(conn) < 0) return -1;
storageDriverLock(); - for (i = 0; i < driver->pools.count && got < nnames; i++) { - virStoragePoolObjPtr obj = driver->pools.objs[i]; - virStoragePoolObjLock(obj); - if (virConnectListDefinedStoragePoolsCheckACL(conn, obj->def) && - !virStoragePoolObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virStoragePoolObjUnlock(obj); - goto cleanup; - } - got++; - } - virStoragePoolObjUnlock(obj); - } + got = virStoragePoolObjGetNames(&driver->pools, conn, false, + virConnectListDefinedStoragePoolsCheckACL, + names, maxnames); storageDriverUnlock(); return got; - - cleanup: - storageDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - memset(names, 0, nnames * sizeof(*names)); - return -1; }
/* This method is required to be re-entrant / thread safe, so diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4fb9915..1a6601e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4130,35 +4130,23 @@ testConnectNumOfStoragePools(virConnectPtr conn) return numActive; }
+ static int testConnectListStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { testDriverPtr privconn = conn->privateData; int n = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names));
;-)
testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->pools.count && n < nnames; i++) { - virStoragePoolObjLock(privconn->pools.objs[i]); - if (virStoragePoolObjIsActive(privconn->pools.objs[i]) && - VIR_STRDUP(names[n++], privconn->pools.objs[i]->def->name) < 0) { - virStoragePoolObjUnlock(privconn->pools.objs[i]); - goto error; - } - virStoragePoolObjUnlock(privconn->pools.objs[i]); - } + n = virStoragePoolObjGetNames(&privconn->pools, conn, true, NULL, + names, maxnames); testDriverUnlock(privconn);
return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; }
@@ -4176,35 +4164,23 @@ testConnectNumOfDefinedStoragePools(virConnectPtr conn) return numInactive; }
+ static int testConnectListDefinedStoragePools(virConnectPtr conn, char **const names, - int nnames) + int maxnames) { testDriverPtr privconn = conn->privateData; int n = 0; - size_t i; + + memset(names, 0, maxnames * sizeof(*names));
;-)
testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->pools.count && n < nnames; i++) { - virStoragePoolObjLock(privconn->pools.objs[i]); - if (!virStoragePoolObjIsActive(privconn->pools.objs[i]) && - VIR_STRDUP(names[n++], privconn->pools.objs[i]->def->name) < 0) { - virStoragePoolObjUnlock(privconn->pools.objs[i]); - goto error; - } - virStoragePoolObjUnlock(privconn->pools.objs[i]); - } + n = virStoragePoolObjGetNames(&privconn->pools, conn, false, NULL, + names, maxnames); testDriverUnlock(privconn);
return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; }
static int
Michal
Alter virStoragePoolObjListExport in order to pass the drivers->pools by reference Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 8 ++++---- src/conf/virstorageobj.h | 2 +- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index a9367a2..34a3225 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1100,7 +1100,7 @@ virStoragePoolMatch(virStoragePoolObjPtr poolobj, int virStoragePoolObjListExport(virConnectPtr conn, - virStoragePoolObjList poolobjs, + virStoragePoolObjListPtr poolobjs, virStoragePoolPtr **pools, virStoragePoolObjListFilter filter, unsigned int flags) @@ -1111,11 +1111,11 @@ virStoragePoolObjListExport(virConnectPtr conn, int ret = -1; size_t i; - if (pools && VIR_ALLOC_N(tmp_pools, poolobjs.count + 1) < 0) + if (pools && VIR_ALLOC_N(tmp_pools, poolobjs->count + 1) < 0) goto cleanup; - for (i = 0; i < poolobjs.count; i++) { - virStoragePoolObjPtr poolobj = poolobjs.objs[i]; + for (i = 0; i < poolobjs->count; i++) { + virStoragePoolObjPtr poolobj = poolobjs->objs[i]; virStoragePoolObjLock(poolobj); if ((!filter || filter(conn, poolobj->def)) && virStoragePoolMatch(poolobj, flags)) { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index b651865..7c11910 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -191,7 +191,7 @@ virStoragePoolObjUnlock(virStoragePoolObjPtr obj); int virStoragePoolObjListExport(virConnectPtr conn, - virStoragePoolObjList poolobjs, + virStoragePoolObjListPtr poolobjs, virStoragePoolPtr **pools, virStoragePoolObjListFilter filter, unsigned int flags); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9493da8..6b8bbb5 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2653,7 +2653,7 @@ storageConnectListAllStoragePools(virConnectPtr conn, goto cleanup; storageDriverLock(); - ret = virStoragePoolObjListExport(conn, driver->pools, pools, + ret = virStoragePoolObjListExport(conn, &driver->pools, pools, virConnectListAllStoragePoolsCheckACL, flags); storageDriverUnlock(); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1a6601e..a2a17ef 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4194,7 +4194,7 @@ testConnectListAllStoragePools(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL, -1); testDriverLock(privconn); - ret = virStoragePoolObjListExport(conn, privconn->pools, pools, + ret = virStoragePoolObjListExport(conn, &privconn->pools, pools, NULL, flags); testDriverUnlock(privconn); -- 2.9.3
Create a couple of helpers that will perform the same call sequence. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 144 ++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 85 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6b8bbb5..8b55acc 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -387,6 +387,56 @@ storageStateCleanup(void) } +static virStoragePoolObjPtr +storagePoolObjFindByUUID(const unsigned char *uuid, + const char *name) +{ + virStoragePoolObjPtr pool; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, uuid))) { + virUUIDFormat(uuid, uuidstr); + if (name) + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching uuid '%s' (%s)"), + uuidstr, name); + else + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching uuid '%s'"), + uuidstr); + } + + return pool; +} + + +static virStoragePoolObjPtr +virStoragePoolObjFromStoragePool(virStoragePoolPtr pool) +{ + virStoragePoolObjPtr ret; + + storageDriverLock(); + ret = storagePoolObjFindByUUID(pool->uuid, pool->name); + storageDriverUnlock(); + + return ret; +} + + +static virStoragePoolObjPtr +storagePoolObjFindByName(const char *name) +{ + virStoragePoolObjPtr pool; + + storageDriverLock(); + if (!(pool = virStoragePoolObjFindByName(&driver->pools, name))) + virReportError(VIR_ERR_NO_STORAGE_POOL, + _("no storage pool with matching name '%s'"), name); + storageDriverUnlock(); + + return pool; +} + static virStoragePoolPtr storagePoolLookupByUUID(virConnectPtr conn, @@ -396,16 +446,10 @@ storagePoolLookupByUUID(virConnectPtr conn, virStoragePoolPtr ret = NULL; storageDriverLock(); - pool = virStoragePoolObjFindByUUID(&driver->pools, uuid); + pool = storagePoolObjFindByUUID(uuid, NULL); storageDriverUnlock(); - - if (!pool) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s'"), uuidstr); + if (!pool) return NULL; - } if (virStoragePoolLookupByUUIDEnsureACL(conn, pool->def) < 0) goto cleanup; @@ -425,15 +469,8 @@ storagePoolLookupByName(virConnectPtr conn, virStoragePoolObjPtr pool; virStoragePoolPtr ret = NULL; - storageDriverLock(); - pool = virStoragePoolObjFindByName(&driver->pools, name); - storageDriverUnlock(); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), name); + if (!(pool = storagePoolObjFindByName(name))) return NULL; - } if (virStoragePoolLookupByNameEnsureACL(conn, pool->def) < 0) goto cleanup; @@ -452,16 +489,8 @@ storagePoolLookupByVolume(virStorageVolPtr vol) virStoragePoolObjPtr pool; virStoragePoolPtr ret = NULL; - storageDriverLock(); - pool = virStoragePoolObjFindByName(&driver->pools, vol->pool); - storageDriverUnlock(); - - if (!pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - vol->pool); + if (!(pool = storagePoolObjFindByName(vol->pool))) return NULL; - } if (virStoragePoolLookupByVolumeEnsureACL(vol->conn, pool->def) < 0) goto cleanup; @@ -588,25 +617,6 @@ storageConnectFindStoragePoolSources(virConnectPtr conn, } -static virStoragePoolObjPtr -virStoragePoolObjFromStoragePool(virStoragePoolPtr pool) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virStoragePoolObjPtr ret; - - storageDriverLock(); - if (!(ret = virStoragePoolObjFindByUUID(&driver->pools, pool->uuid))) { - virUUIDFormat(pool->uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s' (%s)"), - uuidstr, pool->name); - } - storageDriverUnlock(); - - return ret; -} - - static int storagePoolIsActive(virStoragePoolPtr pool) { virStoragePoolObjPtr obj; @@ -813,14 +823,8 @@ storagePoolUndefine(virStoragePoolPtr obj) int ret = -1; storageDriverLock(); - if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s' (%s)"), - uuidstr, obj->name); + if (!(pool = storagePoolObjFindByUUID(obj->uuid, obj->name))) goto cleanup; - } if (virStoragePoolUndefineEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -999,14 +1003,8 @@ storagePoolDestroy(virStoragePoolPtr obj) int ret = -1; storageDriverLock(); - if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s' (%s)"), - uuidstr, obj->name); + if (!(pool = storagePoolObjFindByUUID(obj->uuid, obj->name))) goto cleanup; - } if (virStoragePoolDestroyEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1133,14 +1131,8 @@ storagePoolRefresh(virStoragePoolPtr obj, virCheckFlags(0, -1); storageDriverLock(); - if (!(pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s' (%s)"), - uuidstr, obj->name); + if (!(pool = storagePoolObjFindByUUID(obj->uuid, obj->name))) goto cleanup; - } if (virStoragePoolRefreshEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1283,16 +1275,8 @@ storagePoolSetAutostart(virStoragePoolPtr obj, int ret = -1; storageDriverLock(); - pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); - - if (!pool) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching uuid '%s' (%s)"), - uuidstr, obj->name); + if (!(pool = storagePoolObjFindByUUID(obj->uuid, obj->name))) goto cleanup; - } if (virStoragePoolSetAutostartEnsureACL(obj->conn, pool->def) < 0) goto cleanup; @@ -1713,18 +1697,8 @@ virStorageVolDefFromVol(virStorageVolPtr obj, { virStorageVolDefPtr vol = NULL; - *pool = NULL; - - storageDriverLock(); - *pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); - storageDriverUnlock(); - - if (!*pool) { - virReportError(VIR_ERR_NO_STORAGE_POOL, - _("no storage pool with matching name '%s'"), - obj->pool); + if (!(*pool = storagePoolObjFindByName(obj->pool))) return NULL; - } if (!virStoragePoolObjIsActive(*pool)) { virReportError(VIR_ERR_OPERATION_INVALID, -- 2.9.3
On 04/06/2017 01:48 PM, John Ferlan wrote:
Merge code in storage_driver.c and test_driver into virstorageobj to count the number of volumes/pools, to return a list of names, return the collected list of objects for volumes/pools. For volumes, that's moved code, while for pools that's just changing the export API to take an address of 'devobjs' to follow other usages. I also added code to converge the FindBy{UUID|Name}
FWIW: This is part of the common driver objects code I've been working through. I figured I will post each driver separately rather than one
John Ferlan (7): storage: Introduce virStoragePoolObjNumOfVolumes storage: Introduce virStoragePoolObjVolumeGetNames storage: Introduce virStoragePoolObjVolumeListExport storage: Introduce virStoragePoolObjNumOfStoragePools storage: Introduce virStoragePoolObjGetNames storage: Pass driver arg by ref storage: Create helpers to perform FindByUUID and FindByName
src/conf/virstorageobj.c | 165 +++++++++++++++++++++++- src/conf/virstorageobj.h | 45 ++++++- src/libvirt_private.syms | 5 + src/storage/storage_driver.c | 297 +++++++++++++------------------------------ src/test/test_driver.c | 131 +++++-------------- 5 files changed, 332 insertions(+), 311 deletions(-)
ACK series, but please do see my comments before pushing - you'll need to fix some nits. Michal
participants (2)
-
John Ferlan -
Michal Privoznik