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@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.
>
"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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
