[dpdk-dev] [PATCH 2/7] ethdev: add GTP item

[Adrien Mazarguil]

Hi Beilei,

On Fri, Aug 25, 2017 at 03:50:25PM +0800, Beilei Xing wrote:
> This patch adds GTP items to generic rte flow.
> 
> Signed-off-by: Beilei Xing <beilei.xing at intel.com>

Thanks, several comments below.

> ---
>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>  lib/librte_ether/rte_flow.h        | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 662a912..2843b71 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -955,6 +955,18 @@ Usage example, fuzzy match a TCPv4 packets:
>     | 4     | END      |
>     +-------+----------+
>  
> +Item: ``GTP``
> +^^^^^^^^^^^^^^
> +
> +Matches a GTP header.
> +
> +- ``v_pt_rsv_flags``: version (3b), protocol type (1b), reserved (1b),
> +  extension header flag (1b), sequence number flag (1b), N-PDU number flag(1b).

Missing space after "flag", this line is also too long, you should add a
line break after the last comma.

> +- ``msg_type``: message type.
> +- ``msg_len``: message length.
> +- ``teid``: TEID.
> +- Default ``mask`` matches teid only.
> +
>  Actions
>  ~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index bba6169..73305aa 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -309,6 +309,13 @@ enum rte_flow_item_type {
>  	 * See struct rte_flow_item_fuzzy.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_FUZZY,
> +
> +	/**
> +	 * Matches a GTP header

Missing "." (pedantic mode enabled).

> +	 *
> +	 * See struct rte_flow_item.

Wrong structure, should be rte_flow_item_gtp.

> +	 */
> +	RTE_FLOW_ITEM_TYPE_GTP,
>  };
>  
>  /**
> @@ -735,6 +742,30 @@ static const struct rte_flow_item_fuzzy rte_flow_item_fuzzy_mask = {
>  #endif
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_GTP.
> + *
> + * Matches a GTP header.
> + */
> +struct rte_flow_item_gtp {
> +	/**
> +	 * Version(2b), Protocol type(1b), Reserved(1b),
> +	 * Extension header flag(1b),
> +	 * Sequqnce number flag(1b),
> +	 * N-PDU number flag(1b).
> +	 */

No need to capitalize everything, only the first one. Several missing spaces
betwen name and bit widths ("foo(42b)" => "foo (42b)").

There's also a typo on "Sequqnce".

> +	uint8_t v_pt_rsv_flags;
> +	uint8_t msg_type; /**< Message type */
> +	rte_be16_t msg_len; /**< Message length */

These comments lack ending ".".

> +	rte_be32_t teid;

This field lack a comment, even if obvious, please add it for
consistency. You can use this opportunity to expand the acronym as well.

> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GTP. */
> +#ifndef __cplusplus
> +	static const struct rte_flow_item_gtp rte_flow_item_gtp_mask = {
> +		.teid = RTE_BE32(0xffffffff),
> +	};
> +#endif

Indentation is wrong.

> +/**
>   * Matching pattern item definition.
>   *
>   * A pattern is formed by stacking items starting from the lowest protocol
> -- 
> 2.5.5
> 

It's not a problem if you want to keep them separate, however note that you
could make the testpmd update part of this commit. The API change can be
considered useless without an implementation counterpart.

-- 
Adrien Mazarguil
6WIND