9:06 p.m.
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>
---
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