Re: [dpdk-dev] [RFC PATCH] ethdev: add support for testpmd-compliant flow rule dumping

[Ivan Malov <Ivan.Malov@oktetlabs.ru>]

Hi Stephen,

I agree that the API rte_flow_snprint() itself would look better if it 
provided the number of characters in its return value, like snprintf 
does. However, with respect to all internal helpers, this wouldn't be 
that clear and simple: one would have to update the buffer pointer and 
decrease the buffer size before each internal (smaller) helper 
invocation. That would make the code more cumbersome in many places.

In v2, I will at least try to make the main API return the number of 
characters. Other than that, it can be discussed further.

Thank you.

On 31/05/2021 05:28, Stephen Hemminger wrote:
> On Sun, 30 May 2021 07:27:32 +0000
> Ori Kam <orika@nvidia.com> wrote:
> 
>>>
>>> DPDK applications (for example, OvS) or tests which use RTE flow API need to
>>> log created or rejected flow rules to help to recognise what goes right or
>>> wrong. From this standpoint, testpmd-compliant format is nice for the
>>> purpose because it allows to copy-paste the flow rules and debug using
>>> testpmd.
>>>
>>> Recognisable pattern items:
>>> VOID, VF, PF, PHY_PORT, PORT_ID, ETH, VLAN, IPV4, IPV6, UDP, TCP, VXLAN,
>>> NVGRE, GENEVE, MARK, PPPOES, PPPOED.
>>>
>>> Recognisable actions:
>>> VOID, JUMP, MARK, FLAG, QUEUE, DROP, COUNT, RSS, PF, VF, PHY_PORT,
>>> PORT_ID, OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID,
>>> OF_SET_VLAN_PCP, VXLAN_ENCAP, VXLAN_DECAP.
>>>
>>> Recognisable RSS types (action RSS):
>>> IPV4, FRAG_IPV4, NONFRAG_IPV4_TCP, NONFRAG_IPV4_UDP,
>>> NONFRAG_IPV4_OTHER, IPV6, FRAG_IPV6, NONFRAG_IPV6_TCP,
>>> NONFRAG_IPV6_UDP, NONFRAG_IPV6_OTHER, IPV6_EX, IPV6_TCP_EX,
>>> IPV6_UDP_EX, L3_SRC_ONLY, L3_DST_ONLY, L4_SRC_ONLY, L4_DST_ONLY.
>>>
>>> Unrecognised parts of the flow specification are represented by tokens
>>> "{unknown}" and "{unknown bits}". Interested parties are welcome to
>>> extend this tool to recognise more items and actions.
>>>
>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>> ---
>>>   lib/ethdev/meson.build        |    1 +
>>>   lib/ethdev/rte_flow.h         |   33 +
>>>   lib/ethdev/rte_flow_snprint.c | 1681
>>> +++++++++++++++++++++++++++++++++
>>>   lib/ethdev/version.map        |    3 +
>>>   4 files changed, 1718 insertions(+)
>>>   create mode 100644 lib/ethdev/rte_flow_snprint.c
>>>
>>> diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build index
>>> 0205c853df..97bba4fa1b 100644
>>> --- a/lib/ethdev/meson.build
>>> +++ b/lib/ethdev/meson.build
>>> @@ -8,6 +8,7 @@ sources = files(
>>>           'rte_class_eth.c',
>>>           'rte_ethdev.c',
>>>           'rte_flow.c',
>>> +	'rte_flow_snprint.c',
>>>           'rte_mtr.c',
>>>           'rte_tm.c',
>>>   )
>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
>>> 961a5884fe..cd5e9ef631 100644
>>> --- a/lib/ethdev/rte_flow.h
>>> +++ b/lib/ethdev/rte_flow.h
>>> @@ -4288,6 +4288,39 @@ rte_flow_tunnel_item_release(uint16_t port_id,
>>>   			     struct rte_flow_item *items,
>>>   			     uint32_t num_of_items,
>>>   			     struct rte_flow_error *error);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>> + * Dump testpmd-compliant textual representation of the flow rule.
>>> + * Invoke this with zero-size buffer to learn the string size and
>>> + * invoke this for the second time to actually dump the flow rule.
>>> + * The buffer size on the second invocation = the string size + 1.
>>> + *
>>> + * @param[out] buf
>>> + *   Buffer to save the dump in, or NULL
>>> + * @param buf_size
>>> + *   Buffer size, or 0
>>> + * @param[out] nb_chars_total
>>> + *   Resulting string size (excluding the terminating null byte)
>>> + * @param[in] attr
>>> + *   Flow rule attributes.
>>> + * @param[in] pattern
>>> + *   Pattern specification (list terminated by the END pattern item).
>>> + * @param[in] actions
>>> + *   Associated actions (list terminated by the END action).
>>> + *
>>> + * @return
>>> + *   0 on success, a negative errno value otherwise
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_flow_snprint(char *buf, size_t buf_size, size_t *nb_chars_total,
>>> +		 const struct rte_flow_attr *attr,
>>> +		 const struct rte_flow_item pattern[],
>>> +		 const struct rte_flow_action actions[]);
>>> +
> 
> The code would be clearer and simpler if you adopted the same return value
> as snprintf. Then lots of places could be just tail calls and the nb_chars_total
> would be unnecessary.
> 

-- 
Ivan M