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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux