[ovs-dev] [PATCH 3/3] odp-util: Use ovs_key_attr_to_string() names in format_odp_key_attr().

Ben Pfaff blp at nicira.com
Mon Nov 7 21:24:36 UTC 2011


On Mon, Nov 07, 2011 at 10:53:15AM -0800, Jesse Gross wrote:
> On Mon, Nov 7, 2011 at 9:16 AM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 197b2d0..00d6f36 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > +static void
> > +format_ovs_key_attr_name(struct ds *ds, enum ovs_key_attr attr)
> > +{
> > + ?? ??const char *name = ovs_key_attr_to_string(attr);
> > + ?? ??if (name != ovs_key_attr_unknown) {
> > + ?? ?? ?? ??ds_put_cstr(ds, name);
> > + ?? ??} else {
> > + ?? ?? ?? ??ds_put_format(ds, "key%u", (unsigned int) attr);
> 
> It seems to me that maybe it's better to just always return key%u from
> the original function, I don't know that we ever just want "unknown".

OK.  Fair enough.

> > ??format_odp_pop_action(uint16_t type, struct ds *ds)
> > ??{
> > - ?? ??if (type <= OVS_KEY_ATTR_MAX) {
> > - ?? ?? ?? ??ds_put_format(ds, "pop(%s)", ovs_key_attr_to_string(type));
> > - ?? ??} else {
> > - ?? ?? ?? ??ds_put_format(ds, "pop(key%"PRIu16")", type);
> > - ?? ??}
> > + ?? ??ds_put_cstr(ds, "pop(");
> > + ?? ??format_ovs_key_attr_name(ds, type);
> > + ?? ??ds_put_char(ds, ')');
> 
> We probably can drop this down in the main function down since it's
> pretty simple and is now the same as push/set.

OK.

> > @@ -354,6 +364,7 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
> > ?? ?? const struct ovs_key_nd *nd_key;
> > ?? ?? enum ovs_key_attr attr = nl_attr_type(a);
> >
> > + ?? ??format_ovs_key_attr_name(ds, attr);
> > ?? ?? if (nl_attr_get_size(a) != odp_flow_key_attr_len(nl_attr_type(a))) {
> > ?? ?? ?? ?? ds_put_format(ds, "bad length %zu, expected %d for: ",
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? nl_attr_get_size(a),
> 
> I think you now want a space between the type and "bad".

Yes, you're right, the formatting here would be bad.

Here are redone versions of the initial patch and then of this one.  I
can repost the whole thing if you'd prefer.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Mon, 7 Nov 2011 13:13:36 -0800
Subject: [PATCH] odp-util: New function ovs_key_attr_to_string().

This seems like a worthwhile improvement in itself, but it will also see
additional users in upcoming commits.
---
 lib/odp-util.c |   38 ++++++++++++++++++++++++++++++++------
 1 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1e9289a..433a2b1 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -75,6 +75,36 @@ odp_action_len(uint16_t type)
     return -1;
 }
 
+static const char *
+ovs_key_attr_to_string(enum ovs_key_attr attr)
+{
+    static char unknown_attr[3 + INT_STRLEN(unsigned int) + 1];
+
+    switch (attr) {
+    case OVS_KEY_ATTR_UNSPEC: return "unspec";
+    case OVS_KEY_ATTR_PRIORITY: return "priority";
+    case OVS_KEY_ATTR_TUN_ID: return "tun_id";
+    case OVS_KEY_ATTR_IN_PORT: return "in_port";
+    case OVS_KEY_ATTR_ETHERNET: return "eth";
+    case OVS_KEY_ATTR_8021Q: return "vlan";
+    case OVS_KEY_ATTR_ETHERTYPE: return "eth_type";
+    case OVS_KEY_ATTR_IPV4: return "ipv4";
+    case OVS_KEY_ATTR_IPV6: return "ipv6";
+    case OVS_KEY_ATTR_TCP: return "tcp";
+    case OVS_KEY_ATTR_UDP: return "udp";
+    case OVS_KEY_ATTR_ICMP: return "icmp";
+    case OVS_KEY_ATTR_ICMPV6: return "icmpv6";
+    case OVS_KEY_ATTR_ARP: return "arp";
+    case OVS_KEY_ATTR_ND: return "nd";
+
+    case __OVS_KEY_ATTR_MAX:
+    default:
+        snprintf(unknown_attr, sizeof unknown_attr, "key%u",
+                 (unsigned int) attr);
+        return unknown_attr;
+    }
+}
+
 static void
 format_generic_odp_action(struct ds *ds, const struct nlattr *a)
 {
@@ -164,7 +194,6 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr)
     ds_put_char(ds, ')');
 }
 
-
 static void
 format_odp_action(struct ds *ds, const struct nlattr *a)
 {
@@ -198,11 +227,8 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
         ds_put_cstr(ds, ")");
         break;
     case OVS_ACTION_ATTR_POP:
-        if (nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q) {
-            ds_put_cstr(ds, "pop(vlan)");
-        } else {
-            ds_put_format(ds, "pop(key%"PRIu16")", nl_attr_get_u16(a));
-        }
+        ds_put_format(ds, "pop(%s)",
+                      ovs_key_attr_to_string(nl_attr_get_u16(a)));
         break;
     case OVS_ACTION_ATTR_SAMPLE:
         format_odp_sample_action(ds, a);
-- 
1.7.4.4

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Mon, 7 Nov 2011 13:19:38 -0800
Subject: [PATCH] odp-util: Use ovs_key_attr_to_string() names in
 format_odp_key_attr().

---
 lib/odp-util.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 17134b0..d6a447d 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -298,13 +298,10 @@ odp_flow_key_attr_len(uint16_t type)
     return -1;
 }
 
-
 static void
 format_generic_odp_key(const struct nlattr *a, struct ds *ds)
 {
     size_t len = nl_attr_get_size(a);
-
-    ds_put_format(ds, "key%"PRId16, nl_attr_type(a));
     if (len) {
         const uint8_t *unspec;
         unsigned int i;
@@ -349,8 +346,9 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
     const struct ovs_key_nd *nd_key;
     enum ovs_key_attr attr = nl_attr_type(a);
 
+    ds_put_cstr(ds, ovs_key_attr_to_string(attr));
     if (nl_attr_get_size(a) != odp_flow_key_attr_len(nl_attr_type(a))) {
-        ds_put_format(ds, "bad length %zu, expected %d for: ",
+        ds_put_format(ds, "(bad length %zu, expected %d)",
                       nl_attr_get_size(a),
                       odp_flow_key_attr_len(nl_attr_type(a)));
         format_generic_odp_key(a, ds);
@@ -359,27 +357,27 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
 
     switch (attr) {
     case OVS_KEY_ATTR_PRIORITY:
-        ds_put_format(ds, "priority(%"PRIu32")", nl_attr_get_u32(a));
+        ds_put_format(ds, "(%"PRIu32")", nl_attr_get_u32(a));
         break;
 
     case OVS_KEY_ATTR_TUN_ID:
-        ds_put_format(ds, "tun_id(%#"PRIx64")", ntohll(nl_attr_get_be64(a)));
+        ds_put_format(ds, "(%#"PRIx64")", ntohll(nl_attr_get_be64(a)));
         break;
 
     case OVS_KEY_ATTR_IN_PORT:
-        ds_put_format(ds, "in_port(%"PRIu32")", nl_attr_get_u32(a));
+        ds_put_format(ds, "(%"PRIu32")", nl_attr_get_u32(a));
         break;
 
     case OVS_KEY_ATTR_ETHERNET:
         eth_key = nl_attr_get(a);
-        ds_put_format(ds, "eth(src="ETH_ADDR_FMT",dst="ETH_ADDR_FMT")",
+        ds_put_format(ds, "(src="ETH_ADDR_FMT",dst="ETH_ADDR_FMT")",
                       ETH_ADDR_ARGS(eth_key->eth_src),
                       ETH_ADDR_ARGS(eth_key->eth_dst));
         break;
 
     case OVS_KEY_ATTR_8021Q:
         q_key = nl_attr_get(a);
-        ds_put_cstr(ds, "vlan(");
+        ds_put_cstr(ds, "(");
         if (q_key->q_tpid != htons(ETH_TYPE_VLAN)) {
             ds_put_format(ds, "tpid=0x%04"PRIx16",", ntohs(q_key->q_tpid));
         }
@@ -389,13 +387,13 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
         break;
 
     case OVS_KEY_ATTR_ETHERTYPE:
-        ds_put_format(ds, "eth_type(0x%04"PRIx16")",
+        ds_put_format(ds, "(0x%04"PRIx16")",
                       ntohs(nl_attr_get_be16(a)));
         break;
 
     case OVS_KEY_ATTR_IPV4:
         ipv4_key = nl_attr_get(a);
-        ds_put_format(ds, "ipv4(src="IP_FMT",dst="IP_FMT","
+        ds_put_format(ds, "(src="IP_FMT",dst="IP_FMT","
                       "proto=%"PRId8",tos=%"PRIu8",frag=%s)",
                       IP_ARGS(&ipv4_key->ipv4_src),
                       IP_ARGS(&ipv4_key->ipv4_dst),
@@ -411,7 +409,7 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
         inet_ntop(AF_INET6, ipv6_key->ipv6_src, src_str, sizeof src_str);
         inet_ntop(AF_INET6, ipv6_key->ipv6_dst, dst_str, sizeof dst_str);
 
-        ds_put_format(ds, "ipv6(src=%s,dst=%s,proto=%"PRId8",tos=%"PRIu8","
+        ds_put_format(ds, "(src=%s,dst=%s,proto=%"PRId8",tos=%"PRIu8","
                       "frag=%s)",
                       src_str, dst_str, ipv6_key->ipv6_proto,
                       ipv6_key->ipv6_tos,
@@ -421,31 +419,31 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
 
     case OVS_KEY_ATTR_TCP:
         tcp_key = nl_attr_get(a);
-        ds_put_format(ds, "tcp(src=%"PRIu16",dst=%"PRIu16")",
+        ds_put_format(ds, "(src=%"PRIu16",dst=%"PRIu16")",
                       ntohs(tcp_key->tcp_src), ntohs(tcp_key->tcp_dst));
         break;
 
     case OVS_KEY_ATTR_UDP:
         udp_key = nl_attr_get(a);
-        ds_put_format(ds, "udp(src=%"PRIu16",dst=%"PRIu16")",
+        ds_put_format(ds, "(src=%"PRIu16",dst=%"PRIu16")",
                       ntohs(udp_key->udp_src), ntohs(udp_key->udp_dst));
         break;
 
     case OVS_KEY_ATTR_ICMP:
         icmp_key = nl_attr_get(a);
-        ds_put_format(ds, "icmp(type=%"PRIu8",code=%"PRIu8")",
+        ds_put_format(ds, "(type=%"PRIu8",code=%"PRIu8")",
                       icmp_key->icmp_type, icmp_key->icmp_code);
         break;
 
     case OVS_KEY_ATTR_ICMPV6:
         icmpv6_key = nl_attr_get(a);
-        ds_put_format(ds, "icmpv6(type=%"PRIu8",code=%"PRIu8")",
+        ds_put_format(ds, "(type=%"PRIu8",code=%"PRIu8")",
                       icmpv6_key->icmpv6_type, icmpv6_key->icmpv6_code);
         break;
 
     case OVS_KEY_ATTR_ARP:
         arp_key = nl_attr_get(a);
-        ds_put_format(ds, "arp(sip="IP_FMT",tip="IP_FMT",op=%"PRIu16","
+        ds_put_format(ds, "(sip="IP_FMT",tip="IP_FMT",op=%"PRIu16","
                       "sha="ETH_ADDR_FMT",tha="ETH_ADDR_FMT")",
                       IP_ARGS(&arp_key->arp_sip), IP_ARGS(&arp_key->arp_tip),
                       ntohs(arp_key->arp_op), ETH_ADDR_ARGS(arp_key->arp_sha),
@@ -458,7 +456,7 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
         nd_key = nl_attr_get(a);
         inet_ntop(AF_INET6, nd_key->nd_target, target, sizeof target);
 
-        ds_put_format(ds, "nd(target=%s", target);
+        ds_put_format(ds, "(target=%s", target);
         if (!eth_addr_is_zero(nd_key->nd_sll)) {
             ds_put_format(ds, ",sll="ETH_ADDR_FMT,
                           ETH_ADDR_ARGS(nd_key->nd_sll));
-- 
1.7.4.4




More information about the dev mailing list