[Libvirt-cim] [PATCH V5 08/15] DevicePool, reimplement get_diskpool_config with libvirt
Wenchao Xia
xiawenc at linux.vnet.ibm.com
Sun Mar 24 08:31:25 UTC 2013
于 2013-3-22 2:03, John Ferlan 写道:
> On 03/20/2013 11:39 PM, Wenchao Xia wrote:
>> Original implemetion may return pools with NULL name if
>> some pool disappear between two libvirt pool API call. And
>> originally it return the number of pools and negative value
>> when error happens, but caller of this function consider
>> number = 0 as error.
>> As a fix, this patch changed the function prototype, it do
>> not return the pool number anymore, it returns 0 on success
>> and negative on fail now. Code for checking the risk of returning
>> pools with NULL name is also added.
>> Another small fix is, return false in get_disk_parent() when
>> strdup fail.
>>
>> Signed-off-by: Wenchao Xia <xiawenc at linux.vnet.ibm.com>
>> ---
>> src/Virt_DevicePool.c | 176 +++++++++++++++++++++++++++++++++----------------
>> 1 files changed, 120 insertions(+), 56 deletions(-)
>>
>> diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c
>> index 08677e2..185e3cc 100644
>> --- a/src/Virt_DevicePool.c
>> +++ b/src/Virt_DevicePool.c
>> @@ -51,6 +51,22 @@ struct tmp_disk_pool {
>> bool primordial;
>> };
>>
>> +static void free_diskpool(struct tmp_disk_pool *pools, int count)
>> +{
>> + int i;
>> +
>> + if (pools == NULL) {
>> + return;
>> + }
>> +
>> + for (i = 0; i < count; i++) {
>> + free(pools[i].tag);
>> + free(pools[i].path);
>> + }
>> +
>> + free(pools);
>> +}
>> +
>> /*
>> * Right now, detect support and use it, if available.
>> * Later, this can be a configure option if needed
>> @@ -78,6 +94,10 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools,
>> }
>>
>> pools[count].tag = strdup("0");
>> + if (pools[count].tag == NULL) {
>> + count++;
>> + goto free;
>> + }
>> pools[count].path = NULL;
>> pools[count].primordial = true;
>> count++;
>> @@ -85,12 +105,17 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools,
>> *_count = count;
>> *_pools = pools;
>> ret = true;
>> + goto out;
>>
>> + free:
>> + free_diskpool(pools, count);
>> + /* old pool is invalid, update it */
>> + *_count = 0;
>> + *_pools = NULL;
>> out:
>> return ret;
>> }
>>
>> -
>> #if VIR_USE_LIBVIRT_STORAGE
>> int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool)
>> {
>> @@ -117,52 +142,82 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool)
>> return ret;
>> }
>>
>> +/* This function returns 0 on sucess, negative on fail. */
>> static int get_diskpool_config(virConnectPtr conn,
>> - struct tmp_disk_pool **_pools)
>> + struct tmp_disk_pool **_pools,
>> + int *_count)
>> {
>> - int count = 0;
>> + int count = 0, realcount = 0;
>> int i;
>> char ** names = NULL;
>> struct tmp_disk_pool *pools = NULL;
>> + int ret = 0;
>> + bool bret;
>>
>> count = virConnectNumOfStoragePools(conn);
>> - if (count <= 0)
>> + if (count < 0) {
>> + ret = count;
>> goto out;
>> + } else if (count == 0) {
>> + goto set_parent;
>> + }
>>
>> names = calloc(count, sizeof(char *));
>> if (names == NULL) {
>> CU_DEBUG("Failed to alloc space for %i pool names", count);
>> - count = 0;
>> + ret = -1;
>> goto out;
>> }
>>
>> - if (virConnectListStoragePools(conn, names, count) == -1) {
>> - CU_DEBUG("Failed to get storage pools");
>> - count = 0;
>> - goto out;
>> + realcount = virConnectListStoragePools(conn, names, count);
>> + if (realcount < 0) {
>> + CU_DEBUG("Failed to get storage pools, return %d.", realcount);
>> + ret = realcount;
>> + goto free_names;
>> + }
>> + if (realcount == 0) {
>> + CU_DEBUG("Zero pools got, but prelist is %d.", count);
>> + goto set_parent;
>> }
>>
>> - pools = calloc(count, sizeof(*pools));
>> + pools = calloc(realcount, sizeof(*pools));
>> if (pools == NULL) {
>> - CU_DEBUG("Failed to alloc space for %i pool structs", count);
>> - goto out;
>> + CU_DEBUG("Failed to alloc space for %i pool structs",
>> + realcount);
>> + ret = -2;
>> + goto free_names;
>> }
>>
>> - for (i = 0; i < count; i++) {
>> - pools[i].tag = strdup(names[i]);
>> + for (i = 0; i < realcount; i++) {
>> + pools[i].tag = names[i];
>> + names[i] = NULL;
>> pools[i].primordial = false;
>> }
>>
>> - out:
>> - for (i = 0; i < count; i++)
>> - free(names[i]);
>> - free(names);
>> -
>> - get_disk_parent(&pools, &count);
>> + set_parent:
>> + bret = get_disk_parent(&pools, &realcount);
>> + if (bret != true) {
>> + CU_DEBUG("Failed in adding parentpool.");
>> + ret = -4;
>> + goto free_pools;
>
> If here, we will have already called free_diskpools() which is probably
> a good thing since at this point pools and realcount will be inaccurate.
> So just jump to free_names instead (also leave a reason why!!!)
>
OK, I'll add comment in get_disk_parent() that pools will
be freed on fail.
>> + }
>>
>> + /* succeed */
>> *_pools = pools;
>> + *_count = realcount;
>> + goto free_names;
>> +
>> + free_pools:
>> + free_diskpool(pools, realcount);
>>
>> - return count;
>> + free_names:
>> + for (i = 0; i < count; i++) {
>> + free(names[i]);
>> + }
>> + free(names);
>> +
>> + out:
>> + return ret;
>> }
>>
>> static bool diskpool_set_capacity(virConnectPtr conn,
>> @@ -294,42 +349,59 @@ static int parse_diskpool_line(struct tmp_disk_pool *pool,
>> return (ret == 2);
>> }
>>
>> +/* return 0 on sucess, negative on fail. */
>> static int get_diskpool_config(virConnectPtr conn,
>> - struct tmp_disk_pool **_pools)
>> + struct tmp_disk_pool **_pools,
>> + int *_count)
>> {
>> const char *path = DISK_POOL_CONFIG;
>> FILE *config;
>> char *line = NULL;
>> size_t len = 0;
>> - int count = 0;
>> - struct tmp_disk_pool *pools = NULL;
>> + int count = 0, ret = 0;
>> + struct tmp_disk_pool *pools = NULL, *new_pools = NULL;
>> + bool bret;
>>
>> config = fopen(path, "r");
>> if (config == NULL) {
>> CU_DEBUG("Failed to open %s: %m", path);
>> - return 0;
>> + ret = -1;
>> + goto out;
>> }
>>
>> while (getline(&line, &len, config) > 0) {
>> - pools = realloc(pools,
>> - (count + 1) * (sizeof(*pools)));
>> - if (pools == NULL) {
>> + new_pools = realloc(pools,
>> + (count + 1) * (sizeof(*pools)));
>> + if (new_pools == NULL) {
>> CU_DEBUG("Failed to alloc new pool");
>> - goto out;
>> + ret = -2;
>> + goto free_pools;
>> }
>> + pools = new_pools;
>
> Once the realloc is successful, then pools will have count+1 elems. If
> for some reason the next line files, then I don't think we need to
> realloc again in this loop. Makes the logic a bit more messy though.
>
I'll re-code this old bug like section.
>>
>> if (parse_diskpool_line(&pools[count], line))
>> count++;
>
> Since you have a free(line) below, won't you need a free(line) and line
> = NULL each time through this loop (the line = NULL is so that the
> free(line) below doesn't double deallocate).
>
>> }
>>
>> + bret = get_disk_parent(&pools, &count);
>> + if (bret != true) {
>> + CU_DEBUG("Failed in adding parentpool.");
>> + ret = -3;
>> + goto free_pools;
>
> If here, we will have already called free_diskpools() which is probably
> a good thing since at this point pools and count will be inaccurate. So
> just jump to clean instead (also leave a reason why!!!)
>
>> + }
>>
>> - get_disk_parent(&pools, &count);
>> - out:
>> - free(line);
>> + /* succeed */
>> *_pools = pools;
>> - fclose(config);
>> + *_count = count;
>> + goto clean;
>>
>> - return count;
>> + free_pools:
>> + free_diskpool(pools, count);
>> + clean:
>> + free(line);
>
> Seeing this free(line) made me realize the issue above...
>
> The rest is fine.
>
> John
>
>> + fclose(config);
>> + out:
>> + return ret;
>> }
>>
>> static bool diskpool_set_capacity(virConnectPtr conn,
>> @@ -367,39 +439,23 @@ static bool diskpool_set_capacity(virConnectPtr conn,
>> }
>>
>> static bool _diskpool_is_member(virConnectPtr conn,
>> - const struct disk_pool *pool,
>> + const struct tmp_disk_pool *pool,
>> const char *file)
>> {
>> return STARTS_WITH(file, pool->path);
>> }
>> #endif
>>
>> -static void free_diskpool(struct tmp_disk_pool *pools, int count)
>> -{
>> - int i;
>> -
>> - if (pools == NULL)
>> - return;
>> -
>> - for (i = 0; i < count; i++) {
>> - free(pools[i].tag);
>> - free(pools[i].path);
>> - }
>> -
>> - free(pools);
>> -}
>> -
>> static char *_diskpool_member_of(virConnectPtr conn,
>> const char *file)
>> {
>> struct tmp_disk_pool *pools = NULL;
>> int count;
>> - int i;
>> + int i, ret;
>> char *pool = NULL;
>>
>> - count = get_diskpool_config(conn, &pools);
>> - if (count == 0) {
>> - free(pools);
>> + ret = get_diskpool_config(conn, &pools, &count);
>> + if (ret < 0) {
>> return NULL;
>> }
>>
>> @@ -1088,9 +1144,17 @@ static CMPIStatus diskpool_instance(virConnectPtr conn,
>> CMPIStatus s = {CMPI_RC_OK, NULL};
>> struct tmp_disk_pool *pools = NULL;
>> int count = 0;
>> - int i;
>> + int i, ret;
>>
>> - count = get_diskpool_config(conn, &pools);
>> + ret = get_diskpool_config(conn, &pools, &count);
>> + if (ret < 0) {
>> + CU_DEBUG("Failed to get diskpool config, return is %d.", ret);
>> + cu_statusf(broker, &s,
>> + CMPI_RC_ERR_FAILED,
>> + "Failed to get diskpool config, return is %d.",
>> + ret);
>> + return s;
>> + }
>> if ((id == NULL) && (count == 0)) {
>> CU_DEBUG("No defined DiskPools");
>> free(pools);
>>
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
>
--
Best Regards
Wenchao Xia
More information about the Libvirt-cim
mailing list