On Wed, Jun 13, 2012 at 06:12:16PM -0700, Ethan Jackson wrote:
> A smap is a string to string hash map. It has a cleaner interface
> than shash's which were traditionally used for the same purpose.
> This patch implements the data structure, and changes netdev and
> its providers to use it.
>
> Signed-off-by: Ethan Jackson <[email protected]>
I noticed only one potential bug: Is the logic in smap_get_bool() really
right? I think one of the !s has to be wrong.
Otherwise I have minor comments:
It looks like the comment on netdev_get_qos() is not quite up-to-date
(it still refers to 'data' members).
Ditto for netdev_get_queue().
" * " => " *" here:
+static struct smap_node * smap_find__(const struct smap *, const char *key,
+ size_t key_len, size_t hash);
smap_add() indirectly calculates strlen(key) twice, once in xstrdup(),
once in the hash_bytes() call. You could calculate and save strlen(key)
once and then use xmemdup0() instead of xstrdup().
Ditto for smap_add_format().
Ditto for smap_replace().
smap_clear() duplicates the smap_remove_node() code when I think it
could just call smap_remove_node().
In smap_get_node(), please use "size_t" for key_len. I doubt we'll ever
have a string longer than INT_MAX bytes, but I don't know a reason to
prefer "int" here.
smap.h could use at least a brief comment saying that an smap is a
string-to-string map.
In show_dpif(), in ovs-dpctl.c, the cast to (char *) here should now be
unnecessary:
for (i = 0; i < smap_count(&config); i++) {
const struct smap_node *node = nodes[i];
printf("%c %s=%s", i ? ',' : ':',
node->key, (char *) node->value);
}
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
