1:03 p.m.
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>
---
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);