On Fri, Nov 11, 2011 at 2:26 PM, Ben Pfaff <b...@nicira.com> wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 9786c37..96dd716 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
> const struct nlattr *attr)
[...]
> + if (type > OVS_KEY_ATTR_MAX || attrs & (1ULL << type) ||
> + nla_len(nla) < ovs_key_lens[type])
I don't think that we want to accept keys that are larger than our
length. Although it may be used for extensibility in other places,
this is all exact match and therefore not permissive.
> + if (attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) {
> + swkey->eth.type = nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]);
> + if (ntohs(swkey->eth.type) < 1536)
> + return -EINVAL;
> + attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
> + } else
> + swkey->eth.type = htons(ETH_P_802_2);
I think if one half of the if block uses curly braces then the entire
thing is supposed to.
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index a8da627..9830c12 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +static void
> +log_odp_key_attributes(struct vlog_rate_limit *rl, const char *title,
> + uint32_t attrs,
> + const struct nlattr *key, size_t key_len)
> +{
> + struct ds s;
> + int i;
> +
> + if (VLOG_DROP_WARN(rl)) {
> + return;
> + }
> +
> + ds_init(&s);
> + for (i = 0; i < 32; i++) {
> + if (attrs & (1u << i)) {
> + ds_put_format(&s, " %s", ovs_key_attr_to_string(i));
Don't you want the space after the attribute name?
> static bool
> odp_to_ovs_frag(uint8_t odp_frag, struct flow *flow)
> {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> if (odp_frag > OVS_FRAG_TYPE_LATER) {
> + VLOG_ERR_RL(&rl, "invalid frag %"PRIu8, odp_frag);
Should we prepend some kind of description to these types of error
message? Otherwise, I'm not sure that "invalid frag" provides that
much of a clue to most people.
> +/* We don't use nl_policy_parse() so the .optional members don't matter. */
> +static const struct nl_policy odp_key_policy[] = {
> + [OVS_KEY_ATTR_PRIORITY] = { .type = NL_A_U32 },
> + [OVS_KEY_ATTR_TUN_ID] = { .type = NL_A_BE64 },
> + [OVS_KEY_ATTR_IN_PORT] = { .type = NL_A_U32 },
> + [OVS_KEY_ATTR_ETHERNET] = { NL_POLICY_FOR(struct ovs_key_ethernet) },
> + [OVS_KEY_ATTR_8021Q] = { NL_POLICY_FOR(struct ovs_key_8021q) },
> + [OVS_KEY_ATTR_ETHERTYPE] = { .type = NL_A_BE16 },
> + [OVS_KEY_ATTR_IPV4] = { NL_POLICY_FOR(struct ovs_key_ipv4) },
> + [OVS_KEY_ATTR_IPV6] = { NL_POLICY_FOR(struct ovs_key_ipv6) },
> + [OVS_KEY_ATTR_TCP] = { NL_POLICY_FOR(struct ovs_key_tcp) },
> + [OVS_KEY_ATTR_UDP] = { NL_POLICY_FOR(struct ovs_key_udp) },
> + [OVS_KEY_ATTR_ICMP] = { NL_POLICY_FOR(struct ovs_key_icmp) },
> + [OVS_KEY_ATTR_ICMPV6] = { NL_POLICY_FOR(struct ovs_key_icmpv6) },
> + [OVS_KEY_ATTR_ARP] = { NL_POLICY_FOR(struct ovs_key_arp) },
> + [OVS_KEY_ATTR_ND] = { NL_POLICY_FOR(struct ovs_key_nd) },
> +};
Is there an easy way to combine this with odp_flow_key_attr_len()?
> int
> odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
> struct flow *flow)
[...]
In the kernel we assign the 'none' type for ip_port and dl_type in the
place where we find that there is no attribute for it instead of ahead
of time, which seems a little nicer to me.
> + missing_attrs = present_attrs & ~expected_attrs;
> + if (missing_attrs) {
> + static struct vlog_rate_limit miss_rl = VLOG_RATE_LIMIT_INIT(10, 10);
> + log_odp_key_attributes(&miss_rl, "expected but not present",
> + missing_attrs, key, key_len);
> + return EINVAL;
> + }
>
> - case __OVS_KEY_ATTR_MAX:
> - default:
> - NOT_REACHED();
> + extra_attrs = expected_attrs & ~present_attrs;
> + if (extra_attrs) {
> + static struct vlog_rate_limit extra_rl = VLOG_RATE_LIMIT_INIT(10,
> 10);
> + log_odp_key_attributes(&extra_rl, "present but not expected",
> + extra_attrs, key, key_len);
> + return EINVAL;
Aren't these two conditions reversed?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
