Re: [dpdk-dev] [PATCH v2 1/3] ethdev: add actions to modify TCP header fields

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Dekel,

On Tue, Apr 02, 2019 at 06:13:19PM +0300, Dekel Peled wrote:
> Add actions:
> - INC_TCP_SEQ - Increase sequence number in the outermost TCP header.
> - DEC_TCP_SEQ - Decrease sequence number in the outermost TCP header.
> - INC_TCP_ACK - Increase acknowledgment number in the outermost TCP
> 		header.
> - DEC_TCP_ACK - Decrease acknowledgment number in the outermost TCP
> 		header.
> 
> Original work by Xiaoyu Min.
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
<snip>
> +Action: ``INC_TCP_SEQ``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Increase sequence number in the outermost TCP header.
> +
> +If this action is used without a valid RTE_FLOW_ITEM_TYPE_TCP flow pattern item,
> +behavior is unspecified, depending on PMD implementation.

I still don't agree with the wording as it implies one must combine this
action with the TCP pattern item or else, while one should simply ensure the
presence of TCP traffic somehow. This may be done by a prior filtering rule.

So here's a generic suggestion which could be used with pretty much all
modifying actions (other actions have the same problem and will have to be
fixed as well eventually):

 Using this action on non-matching traffic results in undefined behavior.

This comment applies to all instances in this patch.

<snip>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_INC_TCP_SEQ
> + * RTE_FLOW_ACTION_TYPE_DEC_TCP_SEQ
> + *
> + * Increase/Decrease outermost TCP sequence number
> + */
> +struct rte_flow_action_modify_tcp_seq {
> +	rte_be32_t value; /**< Value to increase/decrease by. */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_INC_TCP_ACK
> + * RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK
> + *
> + * Increase/Decrease outermost TCP acknowledgment number.
> + */
> +struct rte_flow_action_modify_tcp_ack {
> +	rte_be32_t value; /**< Value to increase/decrease by. */
> +};

Thanks for adding experimental tags and comments, however you didn't reply
anything about using a single action, or at least a single structure for
add/sub/set? I'd like to hear your thoughts.

-- 
Adrien Mazarguil
6WIND