It looks like that the not-null-validation of 'map' can be removed in
static function ofputil_name_map_destroy but should be added for
ofputil_port_map_destroy and ofputil_table_map_destroy.
Other than that, this patch looks good to me. Thanks.
Reviewed-by: Yifeng Sun <pkusunyif...@gmail.com>
On Fri, Jan 5, 2018 at 5:00 PM, Ben Pfaff <b...@ovn.org> wrote:
> This shares the infrastructure for mapping port names and numbers. It will
> be used in an upcoming commit.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> include/openvswitch/ofp-util.h | 33 +++++++--
> lib/ofp-util.c | 150 ++++++++++++++++++++++++++++++
> -----------
> 2 files changed, 138 insertions(+), 45 deletions(-)
>
> diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-
> util.h
> index a9e57ed5808d..84b1dbd2756b 100644
> --- a/include/openvswitch/ofp-util.h
> +++ b/include/openvswitch/ofp-util.h
> @@ -43,15 +43,24 @@ union ofp_action;
> struct ofpact_set_field;
> struct vl_mff_map;
>
> -/* Mapping between port numbers and names. */
> -struct ofputil_port_map {
> +/* Name-number mapping.
> + *
> + * This is not exported directly but only through specializations for port
> + * name-number and table name-number mappings. */
> +struct ofputil_name_map {
> struct hmap by_name;
> struct hmap by_number;
> };
> -
> -#define OFPUTIL_PORT_MAP_INITIALIZER(MAP) \
> +#define OFPUTIL_NAME_MAP_INITIALIZER(MAP) \
> { HMAP_INITIALIZER(&(MAP)->by_name), HMAP_INITIALIZER(&(MAP)->by_number)
> }
>
> +/* Mapping between port numbers and names. */
> +struct ofputil_port_map {
> + struct ofputil_name_map map;
> +};
> +#define OFPUTIL_PORT_MAP_INITIALIZER(MAP) \
> + { OFPUTIL_NAME_MAP_INITIALIZER(&(MAP)->map) }
> +
> void ofputil_port_map_init(struct ofputil_port_map *);
> const char *ofputil_port_map_get_name(const struct ofputil_port_map *,
> ofp_port_t);
> @@ -791,6 +800,22 @@ struct ofputil_table_mod_prop_vacancy {
> uint8_t vacancy; /* Current vacancy (%). */
> };
>
> +/* Mapping between table numbers and names. */
> +struct ofputil_table_map {
> + struct ofputil_name_map map;
> +};
> +#define OFPUTIL_TABLE_MAP_INITIALIZER(MAP) \
> + { OFPUTIL_NAME_MAP_INITIALIZER((MAP).map) }
> +
> +void ofputil_table_map_init(struct ofputil_table_map *);
> +const char *ofputil_table_map_get_name(const struct ofputil_table_map *,
> + uint8_t);
> +uint8_t ofputil_table_map_get_number(const struct ofputil_table_map *,
> + const char *name);
> +void ofputil_table_map_put(struct ofputil_table_map *,
> + uint8_t, const char *name);
> +void ofputil_table_map_destroy(struct ofputil_table_map *);
> +
> /* Abstract ofp_table_mod. */
> struct ofputil_table_mod {
> uint8_t table_id; /* ID of the table, 0xff indicates all
> tables. */
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 47f30c7716f3..43122ef09e5d 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -7454,12 +7454,14 @@ ofputil_port_to_string(ofp_port_t port,
> snprintf(namebuf, bufsize, "%"PRIu32, port);
> }
>
> -/* ofputil_port_map. */
> -struct ofputil_port_map_node {
> +/* ofputil_name_map. */
> +
> +struct ofputil_name_map_node {
> struct hmap_node name_node;
> struct hmap_node number_node;
> - ofp_port_t ofp_port; /* Port number. */
> - char *name; /* Port name. */
> +
> + uint32_t number;
> + char *name;
>
> /* OpenFlow doesn't require port names to be unique, although that's
> the
> * only sensible way. However, even in Open vSwitch it's possible
> for two
> @@ -7469,22 +7471,25 @@ struct ofputil_port_map_node {
> * corner case.
> *
> * OpenFlow does require port numbers to be unique. We check for
> duplicate
> - * ports numbers just in case a switch has a bug. */
> + * ports numbers just in case a switch has a bug.
> + *
> + * OpenFlow doesn't require table names to be unique and Open vSwitch
> + * doesn't try to make them unique. */
> bool duplicate;
> };
>
> -void
> -ofputil_port_map_init(struct ofputil_port_map *map)
> +static void
> +ofputil_name_map_init(struct ofputil_name_map *map)
> {
> hmap_init(&map->by_name);
> hmap_init(&map->by_number);
> }
>
> -static struct ofputil_port_map_node *
> -ofputil_port_map_find_by_name(const struct ofputil_port_map *map,
> +static struct ofputil_name_map_node *
> +ofputil_name_map_find_by_name(const struct ofputil_name_map *map,
> const char *name)
> {
> - struct ofputil_port_map_node *node;
> + struct ofputil_name_map_node *node;
>
> HMAP_FOR_EACH_WITH_HASH (node, name_node, hash_string(name, 0),
> &map->by_name) {
> @@ -7495,38 +7500,38 @@ ofputil_port_map_find_by_name(const struct
> ofputil_port_map *map,
> return NULL;
> }
>
> -static struct ofputil_port_map_node *
> -ofputil_port_map_find_by_number(const struct ofputil_port_map *map,
> - ofp_port_t ofp_port)
> +static struct ofputil_name_map_node *
> +ofputil_name_map_find_by_number(const struct ofputil_name_map *map,
> + uint32_t number)
> {
> - struct ofputil_port_map_node *node;
> + struct ofputil_name_map_node *node;
>
> - HMAP_FOR_EACH_IN_BUCKET (node, number_node, hash_ofp_port(ofp_port),
> + HMAP_FOR_EACH_IN_BUCKET (node, number_node, hash_int(number, 0),
> &map->by_number) {
> - if (node->ofp_port == ofp_port) {
> + if (node->number == number) {
> return node;
> }
> }
> return NULL;
> }
>
> -void
> -ofputil_port_map_put(struct ofputil_port_map *map,
> - ofp_port_t ofp_port, const char *name)
> +static void
> +ofputil_name_map_put(struct ofputil_name_map *map,
> + uint32_t number, const char *name)
> {
> - struct ofputil_port_map_node *node;
> + struct ofputil_name_map_node *node;
>
> /* Look for duplicate name. */
> - node = ofputil_port_map_find_by_name(map, name);
> + node = ofputil_name_map_find_by_name(map, name);
> if (node) {
> - if (node->ofp_port != ofp_port) {
> + if (node->number != number) {
> node->duplicate = true;
> }
> return;
> }
>
> /* Look for duplicate number. */
> - node = ofputil_port_map_find_by_number(map, ofp_port);
> + node = ofputil_name_map_find_by_number(map, number);
> if (node) {
> node->duplicate = true;
> return;
> @@ -7534,19 +7539,53 @@ ofputil_port_map_put(struct ofputil_port_map *map,
>
> /* Add new node. */
> node = xmalloc(sizeof *node);
> - hmap_insert(&map->by_number, &node->number_node,
> hash_ofp_port(ofp_port));
> + hmap_insert(&map->by_number, &node->number_node, hash_int(number, 0));
> hmap_insert(&map->by_name, &node->name_node, hash_string(name, 0));
> - node->ofp_port = ofp_port;
> + node->number = number;
> node->name = xstrdup(name);
> node->duplicate = false;
> }
>
> +static void
> +ofputil_name_map_destroy(struct ofputil_name_map *map)
> +{
> + if (map) {
> + struct ofputil_name_map_node *node, *next;
> +
> + HMAP_FOR_EACH_SAFE (node, next, name_node, &map->by_name) {
> + hmap_remove(&map->by_name, &node->name_node);
> + hmap_remove(&map->by_number, &node->number_node);
> + free(node->name);
> + free(node);
> + }
> + hmap_destroy(&map->by_name);
> + hmap_destroy(&map->by_number);
> + }
> +}
> +
> +/* ofputil_port_map. */
> +
> +void
> +ofputil_port_map_init(struct ofputil_port_map *map)
> +{
> + ofputil_name_map_init(&map->map);
> +}
> +
> +void
> +ofputil_port_map_put(struct ofputil_port_map *map,
> + ofp_port_t ofp_port, const char *name)
> +{
> + ofputil_name_map_put(&map->map, ofp_to_u16(ofp_port), name);
> +}
> +
> const char *
> ofputil_port_map_get_name(const struct ofputil_port_map *map,
> ofp_port_t ofp_port)
> {
> - struct ofputil_port_map_node *node
> - = map ? ofputil_port_map_find_by_number(map, ofp_port) : NULL;
> + struct ofputil_name_map_node *node
> + = (map
> + ? ofputil_name_map_find_by_number(&map->map,
> ofp_to_u16(ofp_port))
> + : NULL);
> return node && !node->duplicate ? node->name : NULL;
> }
>
> @@ -7554,26 +7593,55 @@ ofp_port_t
> ofputil_port_map_get_number(const struct ofputil_port_map *map,
> const char *name)
> {
> - struct ofputil_port_map_node *node
> - = map ? ofputil_port_map_find_by_name(map, name) : NULL;
> - return node && !node->duplicate ? node->ofp_port : OFPP_NONE;
> + struct ofputil_name_map_node *node
> + = map ? ofputil_name_map_find_by_name(&map->map, name) : NULL;
> + return node && !node->duplicate ? u16_to_ofp(node->number) :
> OFPP_NONE;
> }
>
> void
> ofputil_port_map_destroy(struct ofputil_port_map *map)
> {
> - if (map) {
> - struct ofputil_port_map_node *node, *next;
> + ofputil_name_map_destroy(&map->map);
> +}
> +
> +
> +/* ofputil_table_map. */
>
> - HMAP_FOR_EACH_SAFE (node, next, name_node, &map->by_name) {
> - hmap_remove(&map->by_name, &node->name_node);
> - hmap_remove(&map->by_number, &node->number_node);
> - free(node->name);
> - free(node);
> - }
> - hmap_destroy(&map->by_name);
> - hmap_destroy(&map->by_number);
> - }
> +void
> +ofputil_table_map_init(struct ofputil_table_map *map)
> +{
> + ofputil_name_map_init(&map->map);
> +}
> +
> +void
> +ofputil_table_map_put(struct ofputil_table_map *map,
> + uint8_t table_id, const char *name)
> +{
> + ofputil_name_map_put(&map->map, table_id, name);
> +}
> +
> +const char *
> +ofputil_table_map_get_name(const struct ofputil_table_map *map,
> + uint8_t table_id)
> +{
> + struct ofputil_name_map_node *node
> + = map ? ofputil_name_map_find_by_number(&map->map, table_id) :
> NULL;
> + return node && !node->duplicate ? node->name : NULL;
> +}
> +
> +uint8_t
> +ofputil_table_map_get_number(const struct ofputil_table_map *map,
> + const char *name)
> +{
> + struct ofputil_name_map_node *node
> + = map ? ofputil_name_map_find_by_name(&map->map, name) : NULL;
> + return node && !node->duplicate ? node->number : UINT8_MAX;
> +}
> +
> +void
> +ofputil_table_map_destroy(struct ofputil_table_map *map)
> +{
> + ofputil_name_map_destroy(&map->map);
> }
>
> /* Stores the group id represented by 's' into '*group_idp'. 's' may be
> an
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
