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

[Dan Smith]

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 at us.ibm.com