On Mon, Nov 07, 2011 at 10:53:15AM -0800, Jesse Gross wrote:
> On Mon, Nov 7, 2011 at 9:16 AM, Ben Pfaff <b...@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 <b...@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 <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
