[ovs-dev] [PATCH 03/31] fixup: Make packet_type formatting more like other fields.

Jan Scheurich jan.scheurich at ericsson.com
Wed Jun 14 18:21:06 UTC 2017


Makes perfect sense to realize packet_type formatting with standard tools. Thanks for simplifying the code.

We have noted one thing, though: Previously the ID of packet types in name space Ethertype were formatted in hex, while with your patch in decimal, i.e. (1.2048) instead of (1,0x800) for IPv4. As the usual formatting for Ethertype fields is hex, we would like to stick to that convention.

The packet_type for Ethernet could still be printed as "eth" as you suggest.
In all other cases I would also prefer hex formatting for the IDs (e.g. special values in the 0xfff_ range), or introduce additional mnemonics.

> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Ben Pfaff
> Sent: Tuesday, 13 June, 2017 00:28
> To: dev at openvswitch.org
> Cc: Ben Pfaff <blp at ovn.org>
> Subject: [ovs-dev] [PATCH 03/31] fixup: Make packet_type formatting more like other fields.
> 
> I don't see value in making packet_type formatting different from other
> fields.
> 
> Also, format_packet_type_masked() added a trailing comma to its output,
> which is surprising, so this patch removes it.
> 
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  lib/flow.c     | 42 ++++++++++++++++++++-----------
>  lib/match.c    |  1 +
>  lib/odp-util.c | 78 ++++++++++++++--------------------------------------------
>  3 files changed, 48 insertions(+), 73 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index b901a077d790..218ba6fa53c7 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -1153,21 +1153,35 @@ format_flags_masked(struct ds *ds, const char *name,
>      }
>  }
> 
> +static void
> +put_u16_masked(struct ds *s, uint16_t value, uint16_t mask)
> +{
> +    if (!mask) {
> +        ds_put_char(s, '*');
> +    } else {
> +        if (value > 9) {
> +            ds_put_format(s, "0x%"PRIx16, value);
> +        } else {
> +            ds_put_format(s, "%"PRIu16, value);
> +        }
> +
> +        if (mask != UINT16_MAX) {
> +            ds_put_format(s, "/0x%"PRIx16, mask);
> +        }
> +    }
> +}
> +
>  void
> -format_packet_type_masked(struct ds *s, const ovs_be32 value, const ovs_be32 mask)
> -{
> -    if (pt_ns_type_be(mask) == 0) {
> -        ds_put_format(s, "packet_type=(%u,*),",
> -                      pt_ns(value));
> -    } else if (pt_ns_type_be(mask) == OVS_BE16_MAX) {
> -        ds_put_format(s, "packet_type=(%u,%#"PRIx16"),",
> -                      pt_ns(value),
> -                      pt_ns_type(value));
> -    } else{
> -        ds_put_format(s, "packet_type=(%u,%#"PRIx16"/%#"PRIx16"),",
> -                      pt_ns(value),
> -                      pt_ns_type(value),
> -                      pt_ns_type(mask));
> +format_packet_type_masked(struct ds *s, ovs_be32 value, ovs_be32 mask)
> +{
> +    if (value == htonl(PT_ETH) && mask == OVS_BE32_MAX) {
> +        ds_put_cstr(s, "eth");
> +    } else {
> +        ds_put_cstr(s, "packet_type=(");
> +        put_u16_masked(s, pt_ns(value), pt_ns(mask));
> +        ds_put_char(s, ',');
> +        put_u16_masked(s, pt_ns_type(value), pt_ns_type(mask));
> +        ds_put_char(s, ')');
>      }
>  }
> 
> diff --git a/lib/match.c b/lib/match.c
> index 2caf10064bd2..ec27bc117383 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -1258,6 +1258,7 @@ match_format(const struct match *match,
> 
>      if (wc->masks.packet_type) {
>          format_packet_type_masked(s, f->packet_type, wc->masks.packet_type);
> +        ds_put_char(s, ',');
>          if (pt_ns(f->packet_type) == OFPHTN_ETHERTYPE) {
>              dl_type = pt_ns_type_be(f->packet_type);
>          }
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index b57934ba1cd4..798906a337f9 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2954,23 +2954,18 @@ format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
>          break;
> 
>      case OVS_KEY_ATTR_PACKET_TYPE: {
> -        ovs_be32 packet_type = nl_attr_get_be32(a);
> -        uint16_t ns = pt_ns(packet_type);
> -        uint16_t ns_type = pt_ns_type(packet_type);
> +        ovs_be32 value = nl_attr_get_be32(a);
> +        ovs_be32 mask = ma ? nl_attr_get_be32(ma) : OVS_BE32_MAX;
> 
> -        if (!is_exact) {
> -            ovs_be32 mask = nl_attr_get_be32(ma);
> -            uint16_t mask_ns_type = pt_ns_type(mask);
> +        ovs_be16 ns = htons(pt_ns(value));
> +        ovs_be16 ns_mask = htons(pt_ns(mask));
> +        format_be16(ds, "ns", ns, &ns_mask, verbose);
> 
> -            if (mask == 0) {
> -                ds_put_format(ds, "ns=%u,id=*", ns);
> -            } else {
> -                ds_put_format(ds, "ns=%u,id=%#"PRIx16"/%#"PRIx16,
> -                              ns, ns_type, mask_ns_type);
> -            }
> -        } else {
> -            ds_put_format(ds, "ns=%u,id=%#"PRIx16, ns, ns_type);
> -        }
> +        ovs_be16 ns_type = pt_ns_type_be(value);
> +        ovs_be16 ns_type_mask = pt_ns_type_be(mask);
> +        format_be16(ds, "id", ns_type, &ns_type_mask, verbose);
> +
> +        ds_chomp(ds, ',');
>          break;
>      }
> 
> @@ -4313,6 +4308,15 @@ parse_odp_key_mask_attr(const char *s, const struct simap *port_names,
>          SCAN_FIELD("tll=", eth, nd_tll);
>      } SCAN_END(OVS_KEY_ATTR_ND);
> 
> +    struct packet_type {
> +        ovs_be16 ns;
> +        ovs_be16 id;
> +    };
> +    SCAN_BEGIN("packet_type(", struct packet_type) {
> +        SCAN_FIELD("ns=", be16, ns);
> +        SCAN_FIELD("id=", be16, id);
> +    } SCAN_END(OVS_KEY_ATTR_PACKET_TYPE);
> +
>      /* Encap open-coded. */
>      if (!strncmp(s, "encap(", 6)) {
>          const char *start = s;
> @@ -4350,50 +4354,6 @@ parse_odp_key_mask_attr(const char *s, const struct simap *port_names,
>          return s - start;
>      }
> 
> -    /* Packet_type open-coded. */
> -    if (strncmp(s, "packet_type(", strlen("packet_type(")) == 0) {
> -        const char *start = s;
> -        ovs_be32 skey = 0;
> -        ovs_be32 smask = 0;
> -        int len;
> -        uint16_t  ns = 0;
> -        uint16_t  ns_type = 0;
> -
> -        s += strlen("packet_type(");
> -        if (ovs_scan(s, "ns=%"SCNu16",id=%n", &ns, &len)){
> -            if (len == 0) {
> -                return -EINVAL;
> -            }
> -            s += len;
> -            if (strncmp(s, "*", 1) == 0) {
> -                s++;
> -            } else if (ovs_scan(s, "0x%"SCNx16"%n", &ns_type, &len)) {
> -                s += len;
> -                skey = PACKET_TYPE_BE(ns, ns_type);
> -                if ( *s == '/' ) {
> -                    uint16_t  ns_type_mask = 0;
> -                    if (ovs_scan(s, "/0x%"SCNx16"%n", &ns_type_mask, &len)) {
> -                        s += len;
> -                        smask = PACKET_TYPE_BE(ns, ns_type_mask);
> -                    }
> -                }
> -            }
> -
> -            if (*s++ != ')') {
> -                return -EINVAL;
> -            }
> -
> -            nl_msg_put_unspec(key, OVS_KEY_ATTR_PACKET_TYPE, &skey,
> -                              sizeof(skey));
> -            if (mask) {
> -                nl_msg_put_unspec(mask, OVS_KEY_ATTR_PACKET_TYPE, &smask,
> -                                  sizeof(smask));
> -            }
> -        }
> -
> -        return s - start;
> -    }
> -
>      return -EINVAL;
>  }
> 
> --
> 2.10.2
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list