+1 于 2012-6-19 23:35, Eduardo Lima (Etrunko) 写道:

From: "Eduardo Lima (Etrunko)"<eblima@br.ibm.com>

The list pointer was leaking in the case where the list was empty.

Signed-off-by: Eduardo Lima (Etrunko)<eblima@br.ibm.com> --- libxkutil/list_util.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libxkutil/list_util.c b/libxkutil/list_util.c index 84b2ba0..cab0287 100644 --- a/libxkutil/list_util.c +++ b/libxkutil/list_util.c @@ -54,9 +54,12 @@ void list_free(list_t *list) { list_node_t *n, *next;

- if (list == NULL || list->head == NULL) + if (list == NULL) return;

+ if (list->head == NULL) + goto end; + n = list->head;

do { @@ -68,6 +71,7 @@ void list_free(list_t *list) n = next; } while (n != list->head);

+ end: free(list); }

-- Best Regards Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803

On Tue, Jun 19, 2012 at 12:35:39PM -0300, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/acl_parsing.c | 71 +++++++++------------------------------- libxkutil/acl_parsing.h | 5 +-- src/Virt_EntriesInFilterList.c | 49 ++++++++++++++------------- src/Virt_FilterEntry.c | 35 +++++++------------- src/Virt_NestedFilterList.c | 3 -- 5 files changed, 56 insertions(+), 107 deletions(-)

diff --git a/libxkutil/acl_parsing.c b/libxkutil/acl_parsing.c index 41ab319..8156854 100644 --- a/libxkutil/acl_parsing.c +++ b/libxkutil/acl_parsing.c @@ -126,8 +126,6 @@ void cleanup_rule(struct acl_rule *rule)

void cleanup_filter(struct acl_filter *filter) { - int i; - if(filter == NULL) return;

@@ -136,12 +134,7 @@ void cleanup_filter(struct acl_filter *filter) free(filter->chain); free(filter->priority);

- for (i = 0; i < filter->rule_ct; i++) - cleanup_rule(filter->rules[i]); - - free(filter->rules); - filter->rule_ct = 0; - + list_free(filter->rules); list_free(filter->refs); }

@@ -574,53 +567,39 @@ int delete_filter(virConnectPtr conn, struct acl_filter *filter) #endif }

-int append_filter_rule(struct acl_filter *filter, struct acl_rule *rule) +static int filter_rule_cmp(void *list_data, void *user_data) { - struct acl_rule **old_rules = NULL; + struct acl_rule * rule = (struct acl_rule *) list_data; + const char * name = (const char *) user_data;

- if ((filter == NULL) || (rule == NULL)) - return 0; + return strcmp(rule->name, name); +}

- rule->name = make_rule_id(filter->name, filter->rule_ct); - if (rule->name == NULL) +int append_filter_rule(struct acl_filter *filter, struct acl_rule *rule) +{ + if ((filter == NULL) || (rule == NULL)) return 0;

- old_rules = filter->rules; + if (filter->rules == NULL) + filter->rules = list_new((list_data_free_cb) cleanup_rule, + filter_rule_cmp);

- filter->rules = - malloc((filter->rule_ct + 1) * sizeof(struct acl_rule *)); - - if (filter->rules == NULL) { - CU_DEBUG("Failed to allocate memory for new rule"); - filter->rules = old_rules; + rule->name = make_rule_id(filter->name, list_count(filter->rules)); + if (rule->name == NULL) return 0; - } - - memcpy(filter->rules, - old_rules, - filter->rule_ct * sizeof(struct acl_rule *));

- filter->rules[filter->rule_ct] = rule; - filter->rule_ct++; - - free(old_rules); + list_append(filter->rules, rule);

return 1; }

- -static int filter_ref_cmp(void *list_data, void *user_data) -{ - return strcmp((const char *)list_data, (const char *) user_data); -} - int append_filter_ref(struct acl_filter *filter, char *name) { if (filter == NULL || name == NULL) return 0;

if (filter->refs == NULL) - filter->refs = list_new(free, filter_ref_cmp); + filter->refs = list_new(free, (list_data_cmp_cb) strcmp);

if (list_find(filter->refs, name) != NULL) { free(name); @@ -659,24 +638,6 @@ char *make_rule_id(const char *filter, int index)

return rule_id; } - -int parse_rule_id(const char *rule_id, char **filter, int *index) -{ - int ret; - - if ((filter == NULL) || (index == NULL)) - return 0; - ret = sscanf(rule_id, "%as[^:]:%u", filter, index); - if (ret != 2) { - free(*filter); - *filter = NULL; - - return 0; - } - - return 1; -} - /* * Local Variables: * mode: C diff --git a/libxkutil/acl_parsing.h b/libxkutil/acl_parsing.h index a0e2f9a..4dca43f 100644 --- a/libxkutil/acl_parsing.h +++ b/libxkutil/acl_parsing.h @@ -152,9 +152,7 @@ struct acl_filter { char *chain; char *priority;

- struct acl_rule **rules; - int rule_ct; - + list_t *rules; list_t *refs; };

@@ -171,7 +169,6 @@ int get_filter_by_name(virConnectPtr conn, const char *name, struct acl_filter **filter);

char *make_rule_id(const char *filter, int index); -int parse_rule_id(const char *rule_id, char **filter, int *index);

int create_filter(virConnectPtr conn, struct acl_filter *filter); int update_filter(virConnectPtr conn, struct acl_filter *filter); diff --git a/src/Virt_EntriesInFilterList.c b/src/Virt_EntriesInFilterList.c index 2c8ac47..ccaa751 100644 --- a/src/Virt_EntriesInFilterList.c +++ b/src/Virt_EntriesInFilterList.c @@ -45,9 +45,10 @@ static CMPIStatus list_to_rule( CMPIStatus s = {CMPI_RC_OK, NULL}; CMPIInstance *instance = NULL; struct acl_filter *filter = NULL; + struct acl_rule *rule = NULL; const char *name = NULL; virConnectPtr conn = NULL; - int i; + list_node_t *head, *node;

CU_DEBUG("Reference = %s", REF2STR(reference));

@@ -68,21 +69,29 @@ static CMPIStatus list_to_rule( goto out; }

- for (i = 0; i < filter->rule_ct; i++) { - CU_DEBUG("Processing %s", filter->rules[i]->name); + head = node = list_first_node(filter->rules); + if (head == NULL) + goto end; + + do { + rule = list_node_data_get(node); + CU_DEBUG("Processing %s", rule->name);

s = instance_from_rule(_BROKER, info->context, reference, - filter->rules[i], + rule, &instance);

if (instance != NULL) { inst_list_add(list, instance); instance = NULL; } - }

+ node = list_node_next_node(node); + } while (node != head); + + end: cleanup_filters(&filter, 1);

out: @@ -104,8 +113,7 @@ static CMPIStatus rule_to_list( CMPIInstance *instance = NULL; const char *name = NULL; virConnectPtr conn = NULL; - int count = 0; - int i, j = 0; + int i, count = 0;

CU_DEBUG("Reference = %s", REF2STR(reference));

@@ -126,21 +134,18 @@ static CMPIStatus rule_to_list(

/* return the filter that contains the rule */ for (i = 0; i < count; i++) { - for (j = 0; j < filters[i].rule_ct; j++) { - if (STREQC(name, filters[i].rules[j]->name)) { - CU_DEBUG("Processing %s,",filters[i].name); - - s = instance_from_filter(_BROKER, - info->context, - reference, - &filters[i], - &instance); - - if (instance != NULL) { - inst_list_add(list, instance); - instance = NULL; - } - + if (list_find(filters[i].rules, (void *) name) != NULL) { + CU_DEBUG("Processing %s,",filters[i].name); + + s = instance_from_filter(_BROKER, + info->context, + reference, + &filters[i], + &instance); + + if (instance != NULL) { + inst_list_add(list, instance); + instance = NULL; } } } diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 126615b..70f8142 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -596,8 +596,10 @@ CMPIStatus enum_filter_rules( { virConnectPtr conn = NULL; struct acl_filter *filters = NULL; - int i, j, count = 0; + int i, count = 0; CMPIStatus s = {CMPI_RC_OK, NULL}; + list_node_t *head, *node; +

CU_DEBUG("Reference = %s", REF2STR(reference));

@@ -618,11 +620,14 @@ CMPIStatus enum_filter_rules( count = get_filters(conn, &filters);

for (i = 0; i < count; i++) { - for (j = 0; j < filters[i].rule_ct; j++) { - CMPIInstance *instance = NULL; + CMPIInstance *instance = NULL; + head = node = list_first_node(filters[i].rules); + if (head == NULL) + continue;

+ do { instance = convert_rule_to_instance( - filters[i].rules[j], + list_node_data_get(node), broker, context, reference, @@ -630,8 +635,9 @@ CMPIStatus enum_filter_rules(

if (instance != NULL) inst_list_add(list, instance); - }

+ node = list_node_next_node(node); + } while (node != head); }

out: @@ -652,9 +658,7 @@ CMPIStatus get_rule_by_ref( struct acl_rule *rule = NULL; const char *name = NULL; char *filter_name = NULL; - int rule_index; virConnectPtr conn = NULL; - int i;

CU_DEBUG("Reference = %s", REF2STR(reference));

@@ -665,15 +669,6 @@ CMPIStatus get_rule_by_ref( goto out; }

- if (parse_rule_id(name, &filter_name, &rule_index) == 0) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_NOT_FOUND, - "Could not parse filter name"); - goto out; - } - - CU_DEBUG("Filter name = %s, rule index = %u", filter_name, rule_index); - conn = connect_by_classname(_BROKER, CLASSNAME(reference), &s); if (conn == NULL) goto out; @@ -686,13 +681,7 @@ CMPIStatus get_rule_by_ref( goto out; }

- for (i = 0; i < filter->rule_ct; i++) { - if (rule_index == i) { - rule = filter->rules[i]; - break; - } - } - + rule = list_find(filter->rules, (void *) name); if (rule == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_NOT_FOUND, diff --git a/src/Virt_NestedFilterList.c b/src/Virt_NestedFilterList.c index a8565d6..3e9fcc3 100644 --- a/src/Virt_NestedFilterList.c +++ b/src/Virt_NestedFilterList.c @@ -141,9 +141,6 @@ static CMPIStatus parent_to_child( goto out;

/* Walk refs list */ - if (parent_filter->refs == NULL) - goto end; - head = node = list_first_node(parent_filter->refs); if (head == NULL) goto end;

That looks reasonnable. The refactoring also decrease code size, Tentative ACK as I don't know the code, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Tue, Jun 19, 2012 at 12:35:40PM -0300, Eduardo Lima (Etrunko) wrote:

From: "Eduardo Lima (Etrunko)" <eblima@br.ibm.com>

Signed-off-by: Eduardo Lima (Etrunko) <eblima@br.ibm.com> --- libxkutil/pool_parsing.c | 27 ++++++------------------- libxkutil/pool_parsing.h | 4 ++-- libxkutil/xmlgen.c | 29 +++++++++++++++++---------- src/Virt_ResourcePoolConfigurationService.c | 28 +++++++++----------------- 4 files changed, 35 insertions(+), 53 deletions(-)

diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c index e41fc09..bb616d4 100644 --- a/libxkutil/pool_parsing.c +++ b/libxkutil/pool_parsing.c @@ -54,8 +54,6 @@ static void cleanup_net_pool(struct net_pool pool) { }

static void cleanup_disk_pool(struct disk_pool pool) { - uint16_t i; - free(pool.path); free(pool.host); free(pool.src_dir); @@ -63,10 +61,7 @@ static void cleanup_disk_pool(struct disk_pool pool) { free(pool.port_name); free(pool.node_name);

- for (i = 0; i < pool.device_paths_ct; i++) - free(pool.device_paths[i]); - - free(pool.device_paths); + list_free(pool.device_paths); }

void cleanup_virt_pool(struct virt_pool **pool) @@ -130,22 +125,15 @@ static int parse_disk_target(xmlNode *node, struct disk_pool *pool) static int parse_disk_source(xmlNode *node, struct disk_pool *pool) { xmlNode *child; - char **dev_paths = NULL; - unsigned ct = 0;

for (child = node->children; child != NULL; child = child->next) { if (XSTREQ(child->name, "device")) { - char **tmp = NULL; - - tmp = realloc(dev_paths, sizeof(char *) * (ct + 1)); - if (tmp == NULL) { - CU_DEBUG("Could not alloc space for dev path"); - continue; - } - dev_paths = tmp; + if (pool->device_paths == NULL) + pool->device_paths = list_new(free, + (list_data_cmp_cb) strcmp);

- dev_paths[ct] = get_attr_value(child, "path"); - ct++; + list_append(pool->device_paths, + get_attr_value(child, "path")); } else if (XSTREQ(child->name, "host")) { pool->host = get_attr_value(child, "name"); if (pool->host == NULL) @@ -161,12 +149,9 @@ static int parse_disk_source(xmlNode *node, struct disk_pool *pool) } }

- pool->device_paths_ct = ct; - pool->device_paths = dev_paths; return 1;

err: - free(dev_paths); return 0; }

diff --git a/libxkutil/pool_parsing.h b/libxkutil/pool_parsing.h index 9f1a386..50fbeac 100644 --- a/libxkutil/pool_parsing.h +++ b/libxkutil/pool_parsing.h @@ -26,6 +26,7 @@ #include <libvirt/libvirt.h>

#include "../src/svpc_types.h" +#include "list_util.h"

struct net_pool { char *addr; @@ -46,8 +47,7 @@ struct disk_pool { DISK_POOL_LOGICAL, DISK_POOL_SCSI} pool_type; char *path; - char **device_paths; - uint16_t device_paths_ct; + list_t *device_paths; char *host; char *src_dir; char *adapter; diff --git a/libxkutil/xmlgen.c b/libxkutil/xmlgen.c index 2dcd0d2..26e7203 100644 --- a/libxkutil/xmlgen.c +++ b/libxkutil/xmlgen.c @@ -1177,27 +1177,34 @@ static const char *disk_pool_type_to_str(uint16_t type) return NULL; }

+static bool device_paths_foreach(void *list_data, void *user_data) +{ + char *dev_path = (char *) list_data; + xmlNodePtr src = (xmlNodePtr) user_data; + xmlNodePtr tmp = NULL; + + tmp = xmlNewChild(src, NULL, BAD_CAST "device", BAD_CAST NULL); + if (tmp == NULL) + return false; + + if (xmlNewProp(tmp, BAD_CAST "path", BAD_CAST dev_path) == NULL) + return false; + + return true; +} + static const char *set_disk_pool_source(xmlNodePtr disk, struct disk_pool *pool) { xmlNodePtr src; xmlNodePtr tmp; - uint16_t i;

src = xmlNewChild(disk, NULL, BAD_CAST "source", NULL); if (src == NULL) return XML_ERROR;

- for (i = 0; i < pool->device_paths_ct; i++) { - tmp = xmlNewChild(src, NULL, BAD_CAST "device", BAD_CAST NULL); - if (tmp == NULL) - return XML_ERROR; - - if (xmlNewProp(tmp, - BAD_CAST "path", - BAD_CAST pool->device_paths[i]) == NULL) - return XML_ERROR; - } + if (!list_foreach(pool->device_paths, device_paths_foreach, src)) + return XML_ERROR;

if (pool->host != NULL) { tmp = xmlNewChild(src, NULL, BAD_CAST "host", BAD_CAST NULL); diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 751d016..bc3e431 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -143,7 +143,6 @@ static const char *net_rasd_to_pool(CMPIInstance *inst, static void init_disk_pool(struct virt_pool *pool) { pool->pool_info.disk.device_paths = NULL; - pool->pool_info.disk.device_paths_ct = 0; pool->pool_info.disk.path = NULL; pool->pool_info.disk.host = NULL; pool->pool_info.disk.src_dir = NULL; @@ -153,9 +152,8 @@ static void init_disk_pool(struct virt_pool *pool) pool->pool_info.disk.autostart = 0; }

-static char *get_dev_paths(CMPIInstance *inst, - char ***path_list, - uint16_t *count) + +static char *get_dev_paths(CMPIInstance *inst, list_t **path_list) { CMPICount i; CMPICount ct; @@ -170,11 +168,8 @@ static char *get_dev_paths(CMPIInstance *inst, if ((s.rc != CMPI_RC_OK) || (ct <= 0)) return "Unable to get DevicePaths array count";

- *path_list = calloc(ct, sizeof(char *)); if (*path_list == NULL) - return "Failed to alloc space for device paths"; - - *count = ct; + *path_list = list_new(free, (list_data_cmp_cb) strcmp);

for (i = 0; i < ct; i++) { const char *str = NULL; @@ -187,7 +182,7 @@ static char *get_dev_paths(CMPIInstance *inst, if (str == NULL) return "Unable to get value of DevicePaths element";

- *path_list[i] = strdup(str); + list_append(*path_list, strdup(str)); }

return NULL; @@ -198,10 +193,7 @@ static const char *disk_fs_or_disk_or_logical_pool(CMPIInstance *inst, { const char *msg = NULL;

- msg = get_dev_paths(inst, - &pool->pool_info.disk.device_paths, - &pool->pool_info.disk.device_paths_ct); - + msg = get_dev_paths(inst, &pool->pool_info.disk.device_paths);

/* Specifying a value for DevicePaths isn't mandatory for logical pool types. */ @@ -209,7 +201,7 @@ static const char *disk_fs_or_disk_or_logical_pool(CMPIInstance *inst, if (msg != NULL) return msg;

- if (pool->pool_info.disk.device_paths_ct != 1) { + if (list_count(pool->pool_info.disk.device_paths) != 1) { CU_DEBUG("%d only takes one device path", pool->pool_info.disk.pool_type); return "Specified pool type only takes one device path"; @@ -243,14 +235,12 @@ static const char *disk_iscsi_pool(CMPIInstance *inst, const char *val = NULL; const char *msg = NULL;

- msg = get_dev_paths(inst, - &pool->pool_info.disk.device_paths, - &pool->pool_info.disk.device_paths_ct); - + msg = get_dev_paths(inst, &pool->pool_info.disk.device_paths); + if (msg != NULL) return msg;

- if (pool->pool_info.disk.device_paths_ct != 1) + if (list_count(pool->pool_info.disk.device_paths) != 1) return "Specified pool type only takes one device path";

if (cu_get_str_prop(inst, "Host", &val) != CMPI_RC_OK)

Looks fine but I would rename device_paths_foreach() to indicates what the function does instead of how it is used, something like device_path_dump_xml() or something similar, That doesn't affect the code semantic though ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/