netdev - Re: [PATCH 2/4] Move execute_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130409031119.GF11444@verge.net.au>
Date:	Tue, 9 Apr 2013 12:11:19 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
	Isaku Yamahata <yamahata@...inux.co.jp>,
	Ben Pfaff <blp@...ira.com>
Subject: Re: [PATCH 2/4] Move execute_set_action to lib/odp-util.c

On Mon, Apr 08, 2013 at 01:29:52PM -0700, Jesse Gross wrote:
> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@...ge.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 <horms@...ge.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 ?

> 
> > 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()

> > +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?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html