Re: [PATCH 2/4] Move execute_set_action to lib/odp-util.c

[Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>]

On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> Move execute_set_action from lib/dpif-netedev.c to lib/odp-util.c
>
> This is in preparation for using execute_set_action()
> in lib/odp-util.c to handle recirculation/
>
> Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
>
> ---
>
> packet.c might be a better place for execute_set_action()
> but I'm unsure if accessing struct ovs_key_ethernet would
> lead to a layering violation.

I'd be tempted to just put this in it's own file.  As you say, it
doesn't really fit in either of the two existing ones.

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e18e109..ad5873c 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2420,3 +2420,79 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
>      commit_set_priority_action(flow, base, odp_actions);
>      commit_set_skb_mark_action(flow, base, odp_actions);
>  }
> +
> +static void
> +dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)

I think this function should be given a more generic name and possibly
moved to packet.c.

> +void
> +execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
> +                   uint32_t *skb_mark)
> +{
> +    enum ovs_key_attr type = nl_attr_type(a);
> +    const struct ovs_key_ipv4 *ipv4_key;
> +    const struct ovs_key_ipv6 *ipv6_key;
> +    const struct ovs_key_tcp *tcp_key;
> +    const struct ovs_key_udp *udp_key;
> +
> +    switch (type) {
> +    case OVS_KEY_ATTR_PRIORITY:
> +    case OVS_KEY_ATTR_TUNNEL:
> +        /* not implemented */
> +        break;

Don't we need to carry this information along as well similar to skb->mark?

Also, is there a reason to not have the code for push/pop actions here as well?