Re: [Libvirt-cim] [PATCH V5 08/15] DevicePool, reimplement get_diskpool_config with libvirt

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(a)linux.vnet.ibm.com&gt; > --- > 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!!!)

> + } > > + /* 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.

> > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim