Re: [dpdk-dev] [PATCH] examples/flow_filtering: fix example documentation

[Dekel Peled <dekelp@mellanox.com>]

PSB.

> -----Original Message-----
> From: Ori Kam
> Sent: Tuesday, December 25, 2018 5:25 AM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: dev@dpdk.org; Dekel Peled <dekelp@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH] examples/flow_filtering: fix example
> documentation
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> > Sent: Monday, December 24, 2018 12:51 PM
> > To: Ori Kam <orika@mellanox.com>
> > Cc: dev@dpdk.org; Dekel Peled <dekelp@mellanox.com>
> > Subject: [dpdk-dev] [PATCH] examples/flow_filtering: fix example
> > documentation
> >
> > Previous patch removed the VLAN item from example code.
> > This patch fixes the documentation accordingly.
> 
> So why are you modifying the c file?
> 

The doc file quotes the c file, needed to modify both to align doc with code.
Code change includes fix of comments, and removing redundant variables and their initialization.

> >
> > Fixes: 9af4eb565710 ("examples/flow_filtering: remove VLAN item")
> > Cc: orika@mellanox.com
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> >  doc/guides/sample_app_ug/flow_filtering.rst | 74 +++++++------------------
> ----
> >  examples/flow_filtering/flow_blocks.c       | 18 ++-----
> >  2 files changed, 21 insertions(+), 71 deletions(-)
> >
> > diff --git a/doc/guides/sample_app_ug/flow_filtering.rst
> > b/doc/guides/sample_app_ug/flow_filtering.rst
> > index 840d557..9dba85a 100644
> > --- a/doc/guides/sample_app_ug/flow_filtering.rst
> > +++ b/doc/guides/sample_app_ug/flow_filtering.rst
> > @@ -53,7 +53,7 @@ applications and the Environment Abstraction Layer
> > (EAL) options.
> >  Explanation
> >  -----------
> >
> > -The example is build from 2 main files,
> > +The example is built from 2 files,
> >  ``main.c`` which holds the example logic and ``flow_blocks.c`` that
> > holds the  implementation for building the flow rule.
> >
> > @@ -380,13 +380,9 @@ This function is located in the ``flow_blocks.c`` file.
> >     {
> >             struct rte_flow_attr attr;
> >             struct rte_flow_item pattern[MAX_PATTERN_NUM];
> > -           struct rte_flow_action action[MAX_PATTERN_NUM];
> > +           struct rte_flow_action action[MAX_ACTION_NUM];
> >             struct rte_flow *flow = NULL;
> >             struct rte_flow_action_queue queue = { .index = rx_q };
> > -           struct rte_flow_item_eth eth_spec;
> > -           struct rte_flow_item_eth eth_mask;
> > -           struct rte_flow_item_vlan vlan_spec;
> > -           struct rte_flow_item_vlan vlan_mask;
> >             struct rte_flow_item_ipv4 ip_spec;
> >             struct rte_flow_item_ipv4 ip_mask;
> >
> > @@ -404,37 +400,19 @@ This function is located in the ``flow_blocks.c`` file.
> >              * create the action sequence.
> >              * one action only,  move packet to queue
> >              */
> > -
> >             action[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> >             action[0].conf = &queue;
> >             action[1].type = RTE_FLOW_ACTION_TYPE_END;
> >
> >             /*
> > -            * set the first level of the pattern (eth).
> > +            * set the first level of the pattern (ETH).
> >              * since in this example we just want to get the
> >              * ipv4 we set this level to allow all.
> >              */
> > -           memset(&eth_spec, 0, sizeof(struct rte_flow_item_eth));
> > -           memset(&eth_mask, 0, sizeof(struct rte_flow_item_eth));
> > -           eth_spec.type = 0;
> > -           eth_mask.type = 0;
> >             pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> > -           pattern[0].spec = &eth_spec;
> > -           pattern[0].mask = &eth_mask;
> > -
> > -           /*
> > -            * setting the second level of the pattern (vlan).
> > -            * since in this example we just want to get the
> > -            * ipv4 we also set this level to allow all.
> > -            */
> > -           memset(&vlan_spec, 0, sizeof(struct rte_flow_item_vlan));
> > -           memset(&vlan_mask, 0, sizeof(struct rte_flow_item_vlan));
> > -           pattern[1].type = RTE_FLOW_ITEM_TYPE_VLAN;
> > -           pattern[1].spec = &vlan_spec;
> > -           pattern[1].mask = &vlan_mask;
> >
> >             /*
> > -            * setting the third level of the pattern (ip).
> > +            * setting the second level of the pattern (IP).
> >              * in this example this is the level we care about
> >              * so we set it according to the parameters.
> >              */
> > @@ -444,12 +422,12 @@ This function is located in the ``flow_blocks.c`` file.
> >             ip_mask.hdr.dst_addr = dest_mask;
> >             ip_spec.hdr.src_addr = htonl(src_ip);
> >             ip_mask.hdr.src_addr = src_mask;
> > -           pattern[2].type = RTE_FLOW_ITEM_TYPE_IPV4;
> > -           pattern[2].spec = &ip_spec;
> > -           pattern[2].mask = &ip_mask;
> > +           pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
> > +           pattern[1].spec = &ip_spec;
> > +           pattern[1].mask = &ip_mask;
> >
> >             /* the final level must be always type end */
> > -           pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> > +           pattern[2].type = RTE_FLOW_ITEM_TYPE_END;
> >
> >             int res = rte_flow_validate(port_id, &attr, pattern, action, error);
> >             if(!res)
> > @@ -464,14 +442,10 @@ The first part of the function is declaring the
> > structures that will be used.
> >
> >     struct rte_flow_attr attr;
> >     struct rte_flow_item pattern[MAX_PATTERN_NUM];
> > -   struct rte_flow_action action[MAX_PATTERN_NUM];
> > +   struct rte_flow_action action[MAX_ACTION_NUM];
> >     struct rte_flow *flow;
> >     struct rte_flow_error error;
> >     struct rte_flow_action_queue queue = { .index = rx_q };
> > -   struct rte_flow_item_eth eth_spec;
> > -   struct rte_flow_item_eth eth_mask;
> > -   struct rte_flow_item_vlan vlan_spec;
> > -   struct rte_flow_item_vlan vlan_mask;
> >     struct rte_flow_item_ipv4 ip_spec;
> >     struct rte_flow_item_ipv4 ip_mask;
> >
> > @@ -491,33 +465,17 @@ the rule. In this case send the packet to queue.
> >     action[0].conf = &queue;
> >     action[1].type = RTE_FLOW_ACTION_TYPE_END;
> >
> > -The forth part is responsible for creating the pattern and is build
> > from -number of step. In each step we build one level of the pattern
> > starting with
> > +The fourth part is responsible for creating the pattern and is built
> > +from number of steps. In each step we build one level of the pattern
> > +starting with
> >  the lowest one.
> >
> >  Setting the first level of the pattern ETH:
> >
> >  .. code-block:: c
> >
> > -   memset(&eth_spec, 0, sizeof(struct rte_flow_item_eth));
> > -   memset(&eth_mask, 0, sizeof(struct rte_flow_item_eth));
> > -   eth_spec.type = 0;
> > -   eth_mask.type = 0;
> >     pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> > -   pattern[0].spec = &eth_spec;
> > -   pattern[0].mask = &eth_mask;
> > -
> > -Setting the second level of the pattern VLAN:
> > -
> > -.. code-block:: c
> > -
> > -   memset(&vlan_spec, 0, sizeof(struct rte_flow_item_vlan));
> > -   memset(&vlan_mask, 0, sizeof(struct rte_flow_item_vlan));
> > -   pattern[1].type = RTE_FLOW_ITEM_TYPE_VLAN;
> > -   pattern[1].spec = &vlan_spec;
> > -   pattern[1].mask = &vlan_mask;
> >
> > -Setting the third level ip:
> > +Setting the second level of the pattern IP:
> >
> >  .. code-block:: c
> >
> > @@ -527,15 +485,15 @@ Setting the third level ip:
> >     ip_mask.hdr.dst_addr = dest_mask;
> >     ip_spec.hdr.src_addr = htonl(src_ip);
> >     ip_mask.hdr.src_addr = src_mask;
> > -   pattern[2].type = RTE_FLOW_ITEM_TYPE_IPV4;
> > -   pattern[2].spec = &ip_spec;
> > -   pattern[2].mask = &ip_mask;
> > +   pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
> > +   pattern[1].spec = &ip_spec;
> > +   pattern[1].mask = &ip_mask;
> >
> >  Closing the pattern part.
> >
> >  .. code-block:: c
> >
> > -   pattern[3].type = RTE_FLOW_ITEM_TYPE_END;
> > +   pattern[2].type = RTE_FLOW_ITEM_TYPE_END;
> >
> >  The last part of the function is to validate the rule and create it.
> >
> > diff --git a/examples/flow_filtering/flow_blocks.c
> > b/examples/flow_filtering/flow_blocks.c
> > index bae7116..1edf6f9 100644
> > --- a/examples/flow_filtering/flow_blocks.c
> > +++ b/examples/flow_filtering/flow_blocks.c
> > @@ -2,7 +2,8 @@
> >   * Copyright 2017 Mellanox Technologies, Ltd
> >   */
> >
> > -#define MAX_PATTERN_NUM		4
> > +#define MAX_PATTERN_NUM		3
> > +#define MAX_ACTION_NUM		2
> >
> >  struct rte_flow *
> >  generate_ipv4_flow(uint16_t port_id, uint16_t rx_q, @@ -41,11 +42,9
> > @@ struct rte_flow *  {
> >  	struct rte_flow_attr attr;
> >  	struct rte_flow_item pattern[MAX_PATTERN_NUM];
> > -	struct rte_flow_action action[MAX_PATTERN_NUM];
> > +	struct rte_flow_action action[MAX_ACTION_NUM];
> >  	struct rte_flow *flow = NULL;
> >  	struct rte_flow_action_queue queue = { .index = rx_q };
> > -	struct rte_flow_item_eth eth_spec;
> > -	struct rte_flow_item_eth eth_mask;
> >  	struct rte_flow_item_ipv4 ip_spec;
> >  	struct rte_flow_item_ipv4 ip_mask;
> >  	int res;
> > @@ -64,26 +63,19 @@ struct rte_flow *
> >  	 * create the action sequence.
> >  	 * one action only,  move packet to queue
> >  	 */
> > -
> >  	action[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
> >  	action[0].conf = &queue;
> >  	action[1].type = RTE_FLOW_ACTION_TYPE_END;
> >
> >  	/*
> > -	 * set the first level of the pattern (eth).
> > +	 * set the first level of the pattern (ETH).
> >  	 * since in this example we just want to get the
> >  	 * ipv4 we set this level to allow all.
> >  	 */
> > -	memset(&eth_spec, 0, sizeof(struct rte_flow_item_eth));
> > -	memset(&eth_mask, 0, sizeof(struct rte_flow_item_eth));
> > -	eth_spec.type = 0;
> > -	eth_mask.type = 0;
> >  	pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
> > -	pattern[0].spec = &eth_spec;
> > -	pattern[0].mask = &eth_mask;
> >
> >  	/*
> > -	 * setting the third level of the pattern (ip).
> > +	 * setting the second level of the pattern (IP).
> >  	 * in this example this is the level we care about
> >  	 * so we set it according to the parameters.
> >  	 */
> > --
> > 1.8.3.1
> 
> 
> Thanks,
> Ori