Re: [libvirt] [PATCH 2/7] storage: Introduce virStoragePoolObjVolumeGetNames

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(a)redhat.com&gt; > --- > 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