Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure

Pablo Neira Ayuso
2018-11-19T22:59:23Z

On Mon, Nov 19, 2018 at 12:56:23PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:10AM CET, pablo@netfilter.org wrote:
> >This new infrastructure defines the nic actions that you can perform
> >from existing network drivers. This infrastructure allows us to avoid a
> >direct dependency with the native software TC action representation.
> >
> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >---
> >v2: no changes.
> >
> > include/net/flow_dissector.h | 70 ++++++++++++++++++++++++++++++++++++++++++++
> > net/core/flow_dissector.c | 18 ++++++++++++
> > 2 files changed, 88 insertions(+)
> >
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index 965a82b8d881..925c208816f1 100644
> >--- a/include/net/flow_dissector.h
> >+++ b/include/net/flow_dissector.h
> >@@ -402,8 +402,78 @@ void flow_rule_match_enc_keyid(const struct flow_rule *rule,
> > void flow_rule_match_enc_opts(const struct flow_rule *rule,
> > struct flow_match_enc_opts *out);
> >
> >+enum flow_action_key_id {
>
> Why "key"? Why not just "flow_action_id"

Sure, will rename this.

> >+ FLOW_ACTION_KEY_ACCEPT = 0,
> >+ FLOW_ACTION_KEY_DROP,
> >+ FLOW_ACTION_KEY_TRAP,
> >+ FLOW_ACTION_KEY_GOTO,
> >+ FLOW_ACTION_KEY_REDIRECT,
> >+ FLOW_ACTION_KEY_MIRRED,
> >+ FLOW_ACTION_KEY_VLAN_PUSH,
> >+ FLOW_ACTION_KEY_VLAN_POP,
> >+ FLOW_ACTION_KEY_VLAN_MANGLE,
> >+ FLOW_ACTION_KEY_TUNNEL_ENCAP,
> >+ FLOW_ACTION_KEY_TUNNEL_DECAP,
> >+ FLOW_ACTION_KEY_MANGLE,
> >+ FLOW_ACTION_KEY_ADD,
> >+ FLOW_ACTION_KEY_CSUM,
> >+ FLOW_ACTION_KEY_MARK,

I assume I should remove _KEY_ from this enum definitions too.

> >+};
> >+
> >+/* This is mirroring enum pedit_header_type definition for easy mapping between
> >+ * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
> >+ * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
> >+ */
> >+enum flow_act_mangle_base {
>
> Please be consistent in naming: "act" vs "action"

OK.

> >+ FLOW_ACT_MANGLE_UNSPEC = 0,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_ETH,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP4,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP6,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_TCP,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >+};
> >+
> >+struct flow_action_key {
>
> And here "struct flow_action"

OK.

> >+ enum flow_action_key_id id;
> >+ union {
> >+ u32 chain_index; /* FLOW_ACTION_KEY_GOTO */
> >+ struct net_device *dev; /* FLOW_ACTION_KEY_REDIRECT */
> >+ struct { /* FLOW_ACTION_KEY_VLAN */
> >+ u16 vid;
> >+ __be16 proto;
> >+ u8 prio;
> >+ } vlan;
> >+ struct { /* FLOW_ACTION_KEY_PACKET_EDIT */
> >+ enum flow_act_mangle_base htype;
> >+ u32 offset;
> >+ u32 mask;
> >+ u32 val;
> >+ } mangle;
> >+ const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
> >+ u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
> >+ u32 mark; /* FLOW_ACTION_KEY_MARK */
> >+ };
> >+};
> >+
> >+struct flow_action {
>
> And here "struct flow_actions"
>
>
> >+ int num_keys;
>
> unsigned int;

OK.