Original implemetion may return pools with NULL name if some pool disappear between two libvirt pool API call. And caller of this function consider number = 0 as error only but it original return negative value when error before. This patch fix it. V2: Use for instead of while in clean up. Treat zero pool returned from libvirt as normal instead of error. Rebased. v3: fix wrong i++ in for while. v4: changed the function prototype to return negative when error, not return the pool number anymore. In this way code of caller and function in no-use-libvirt-pool case were also changed. Return false in get_disk_parent() when strdup fail. Signed-off-by: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com&gt; --- src/Virt_DevicePool.c | 176 +++++++++++++++++++++++++++++++++--------------- 1 files changed, 121 insertions(+), 55 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 79dc108..bf3dd3b 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -51,6 +51,21 @@ 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 +93,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 +104,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 +141,85 @@ 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++) { + for (i = 0; i < realcount; i++) { pools[i].tag = strdup(names[i]); + if (pools[i].tag == NULL) { + CU_DEBUG("Failed in strdup for name '%s'.", names[i]); + ret = -3; + goto free_pools; + } 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; + } + /* 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 +351,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; if (parse_diskpool_line(&pools[count], line)) count++; } + bret = get_disk_parent(&pools, &count); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -3; + goto free_pools; + } - 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); + fclose(config); + out: + return ret; } static bool diskpool_set_capacity(virConnectPtr conn, @@ -367,39 +441,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; } @@ -1084,9 +1142,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); -- 1.7.1