Re: [dpdk-dev] [PATCH v1 0/7] Flow API helpers enhancements

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

On Wed, Oct 11, 2017 at 07:07:29PM +0100, Ferruh Yigit wrote:
> On 10/11/2017 10:57 AM, Adrien Mazarguil wrote:
> > On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote:
<snip>
> >> After above said, API changes one week before integration deadline, a
> >> new script and make target for automated header file, I am a little
> >> scared :), I will be much relieved to get this in the beginning of the
> >> next release cycle.
> > 
> > I can drop the script from this series to speed up inclusion if it there's
> > any concern about it. It's only a helper to update rte_flow_conv.h after
> > modifying rte_flow.h, I thought it could be useful to anyone, hence I've
> > included it but it's pretty much optional.
> > 
> >> I would like to see more comment on this, specially from PMD maintainers.
> > 
> > Me too. I don't even mind negative ones!
> > 
> > Here's what I plan to do regardless, seeing most concerns so far are with
> > rte_flow_copy()/rte_flow_conv():
> > 
> > - Whether this series is included for 17.11 or later, a v2 is already
> >   necessary.
> > 
> > - I will drop the rte_flow_error() change to submit it instead along another
> >   upcoming series for mlx4 where it's the most needed.
> > 
> > - We'll then continue to discuss rte_flow_conv() as a something nice to have
> >   but not super urgent to integrate and I'll keep trying to convince
> >   everyone it's safe enough.
> > 
> > - Once it becomes clear there's no way to have it for 17.11, I'll update
> >   this series as a somewhat late deprecation notice for rte_flow_copy().
> > 
> > Sounds good?
> 
> OK. Lets get rte_flow_error() change and not block mlx4 changes.
> 
> And I still suggest waiting beginning of the release for rest of the
> patch. So it can come with optional header auto generation.

OK, I will re-submit an updated version later then.

> Related to the rte_flow_error_set() modification, as far as I can see it
> doesn't effect drivers but following drivers are now using it:
> 
> drivers/net/bnxt/
> drivers/net/e1000/
> drivers/net/enic/
> drivers/net/i40e/
> drivers/net/ixgbe/
> drivers/net/mlx4/
> drivers/net/mlx5/
> drivers/net/sfc/
> drivers/net/tap/
> 
> There is already tap and mlx4 fixes in the patch to fix
> "return -rte_flow_error_set(...)" kind of usage.
> 
> Rest uses rte_flow_error_set() as:
> 
> "
> rte_flow_error_set(...);
> return -rte_errno;
> "
> 
> But now it can directly be used as:
> "
> return rte_flow_error_set(...);
> "
> 
> What do you think updating to that usage?

Well, that is actually how I originally envisioned this function to be used,
but for whatever reason I thought a positive return value was the way to go
at the time. Too bad.

> Would you mind updating those drivers as above in a separate patch?

That would be nice but I'm not familiar with most of these PMDs (I'm
only updating mlx4 as part of the RSS resurrection series), and there's
always a risk of breaking something.

The current approach of relying on rte_errno is OK since this function
updates it. I'll put it on my TODO list anyway but given its current size,
it's going to be low priority.

-- 
Adrien Mazarguil
6WIND