On Thu, Oct 20, 2011 at 4:55 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
> +static int validate_action_key(int act_type, const struct nlattr *ovs_key,
> int act_len,
> + const struct sw_flow_key *flow_key)
> +{
I think it's clearer if you just pass the nlattr for the action since
right now you are effectively passing nla_type(), nla_data(), and
nla_len() separately. I think the name act_len is also somewhat
ambiguous as to what it actually measures.
> + int key_type = nla_type(ovs_key);
> +
> + /* There can be only one key in a action */
> + if (nla_total_size(nla_len(ovs_key)) != act_len)
> + return -EINVAL;
> +
> + if (key_type > OVS_KEY_ATTR_MAX ||
> + nla_len(ovs_key) != ovs_key_lens[key_type])
> + return -EINVAL;
> +
> +#define ACTION(act, key) ((act << 8) | key)
Can you #undef this at the end of the function, since it is really
meant to be locally scoped?
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index f78cda2..31c2e27 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> +static void
> dp_netdev_execute_actions(struct dp_netdev *dp,
> struct ofpbuf *packet, struct flow *key,
> const struct nlattr *actions,
[...]
> - case OVS_ACTION_ATTR_POP_VLAN:
> + case OVS_ACTION_ATTR_POP:
> dp_netdev_pop_vlan(packet);
I think Ben wanted an assertion here as well.
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index a471099..9c516bf 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -185,49 +180,30 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
[...]
> + 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));
There's no equals sign after key in the other places we have
formatting of this type.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
