Re: [Libvirt-cim] [PATCH 1 of 4] Add vnc_ports struct and functions for building a list of ports

KR> +#define VNC_PORT_MIN 5900 KR> +#define VNC_PORT_MAX 5999 KR> +#define HASH_SIZE 128 Your hash size is larger than your key domain. That means that unless your hash function is really bad, you should never have any collisions... :) The domain is small enough, that I'd just use an array of MAX-MIN, with a hash function of modulus. KR> +static int resize(struct vnc_ports *list, int newmax) KR> +{ KR> + char **newlist; KR> + int i; KR> + KR> + newlist = realloc(list->list, newmax * sizeof(char *)); KR> + if (!newlist) This should be: if (newlist != NULL) KR> +static void vnc_list_init(struct vnc_ports *vnc_hash[]) KR> +{ KR> + int i; KR> + KR> + for (i = 0; i < HASH_SIZE; i++) { KR> + vnc_hash[i] = malloc(sizeof(struct vnc_ports)); KR> + vnc_hash[i]->list = NULL; Crash here if the above malloc() fails. KR> + vnc_hash[i]->cur = vnc_hash[i]->max = 0; KR> + } KR> +} Since this function can fail, it needs to be non-void and return an indication of success. Alternately, since the array of buckets is static anyway, why not just use a static array instead of allocating memory for all the buckets? KR> +static void vnc_list_free(struct vnc_ports *vnc_hash[]) KR> +{ KR> + int i; KR> + KR> + for (i = 0; i < HASH_SIZE; i++) { You've got a whitespace issue here. KR> + if (vnc_hash[i]->list != NULL) KR> + free(vnc_hash[i]->list); KR> + } You free the list, but you don't free the pointers in the list. Since you're loading those up with the result of strdup() in vnc_list_add(), you leak all of that memory. KR> + vnc_list_init(vnc_hash); This is in vnc_list_free(), but doesn't vnc_list_init() allocate memory for the buckets? This seems to go through and free the list member of each bucket, and then leak the memory for the actual bucket by allocating more memory over top of them. KR> +static int vnc_list_add(struct vnc_ports *list, char *port) KR> +{ KR> + if ((list->cur + 1) >= list->max) { KR> + int ret; KR> + KR> + ret = resize(list, list->max + 5); KR> + KR> + if (!ret) KR> + return 0; KR> + } KR> + KR> + list->list[list->cur] = strdup(port); KR> + list->cur++; KR> + KR> + return 1; KR> +} Any reason not to just use a linked list? Since we're not expecting a lot of ports (100 maximum per system, according to your limits), doing this whole resize thing seems kinda overkill. Inserting at the head if a linked list is easy and cheap. Better yet, why not just keep a count of sessions and avoid all the chaining, collision resolution, and allocation madness? :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com