Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command

[Mordechay Haimovsky <motih@mellanox.com>]

Hi Adrien,
 You are correct, the bug is not where we thought it is, moreover the fix breaks
the CLI and should be rejected.
We investigated more and found that the bug is in the port_flow_query routine
Which passes an incorrect argument to rte_flow_conv as follows:
	     ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
					         &name, sizeof(name), action, &error);
While it should pass onlt the action type as follows:
	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
			    &name, sizeof(name),
			    (void *)(uintptr_t)action->type, &error);
As done in port_flow_list routine (which works)
	if (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
						  &name, sizeof(name),
						  (void *)(uintptr_t)action->type,
						  NULL) <= 0)
And according to the  parameters description of rte_flow_conv_name (called by  rte_flow_conv):
	* @param[in] src
 	*   Depending on @p is_action, source pattern item or action type cast as a
	 *   pointer.

Modifying port_flow_query accordingly solves the issue.
I will issue a new patch tonight.

Moti

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Wednesday, October 10, 2018 11:38 AM
> To: John Daley <johndale@cisco.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command
> 
> Hi John
> 
> On Tue, Oct 09, 2018 at 03:51:24PM -0700, John Daley wrote:
> > This patch fixes the 'flow list <port id>' command which caused a
> > segfault when passing the action or item 'type' field instead of the
> > action or item struct pointer in the call to rte_flow_conv.
> >
> > Fixes: 7d94dcedf7ce ("app/testpmd: rely on flow API conversion
> > function")
> >
> > Signed-off-by: John Daley <johndale@cisco.com>
> 
> That bug was introduced by a broken fix, it wasn't present in the original
> patch, please see yesterday's discussion [1].
> 
> RTE_FLOW_CONV_OP_(ITEM|ACTION)_NAME[_PTR] operations are
> documented as using an integer type (enum rte_flow_item_type) cast as
> (void *) for src because they convert item/action *types* to corresponding
> strings, i.e. no need to allocate temporary items/actions just to retrieve their
> names. I thought it would be more versatile and efficient that way.
> 
> [1] "ethdev: fix flow API item/action name conversion"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> ils.dpdk.org%2Farchives%2Fdev%2F2018-
> October%2F115054.html&amp;data=02%7C01%7Cmotih%40mellanox.com%7
> C76b627327b134664eabe08d62e8bba68%7Ca652971c7d2e4d9ba6a4d149256f
> 461b%7C0%7C0%7C636747575020110564&amp;sdata=nRUfdIhoS%2F2Rm76y
> osDvBlttJlvcGJDB1egot8pql%2FY%3D&amp;reserved=0
> 
> > ---
> >  app/test-pmd/config.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 86c205806..2ce40f3e1 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1354,7 +1354,7 @@ port_flow_list(portid_t port_id, uint32_t n, const
> uint32_t group[n])
> >  		while (item->type != RTE_FLOW_ITEM_TYPE_END) {
> >  			if
> (rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_NAME_PTR,
> >  					  &name, sizeof(name),
> > -					  (void *)(uintptr_t)item->type,
> > +					  (void *)(uintptr_t)item,
> 
> Also, while it does work because type is the first field, it should have read
> "&item->type" for correctness. Anyway this patch shouldn't be needed
> assuming the broken fix is reverted.
> 
> >  					  NULL) <= 0)
> >  				name = "[UNKNOWN]";
> >  			if (item->type != RTE_FLOW_ITEM_TYPE_VOID) @@
> -1365,7 +1365,7 @@
> > port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
> >  		while (action->type != RTE_FLOW_ACTION_TYPE_END) {
> >  			if
> (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> >  					  &name, sizeof(name),
> > -					  (void *)(uintptr_t)action->type,
> > +					  (void *)(uintptr_t)action,
> 
> Ditto.
> 
> >  					  NULL) <= 0)
> >  				name = "[UNKNOWN]";
> >  			if (action->type != RTE_FLOW_ACTION_TYPE_VOID)
> > --
> > 2.16.2
> >
> 
> Thanks.
> 
> --
> Adrien Mazarguil
> 6WIND