On Mon, Apr 8, 2013 at 8:11 PM, Simon Horman <ho...@verge.net.au> wrote:
> On Mon, Apr 08, 2013 at 01:29:52PM -0700, Jesse Gross wrote:
>> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <ho...@verge.net.au> 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 <ho...@verge.net.au>
>> >
>> > ---
>> >
>> > 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.
>
> perhaps execute-action.c ?
Sure.
>>
>> > 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.
>
> Sure, how about eth_set_src_and_dst()
That sounds fine.
>> > +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?
>
> Most likely, sorry for missing that.
>
>> Also, is there a reason to not have the code for push/pop actions here as
>> well?
>
> Good point.
>
> With that in mind perhaps execute_set_or_mpls_action() would
> be a good name for the function?
I'm not sure that this is specific to MPLS. Won't we basically just
have the execute loop from dpif-netdev.c here?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
