[Libvirt-cim] [PATCH V5 08/15] DevicePool, reimplement get_diskpool_config with libvirt
John Ferlan
jferlan at redhat.com
Thu Mar 21 18:03:15 UTC 2013
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!!!)
> + }
>
> + /* 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);
>
More information about the Libvirt-cim
mailing list