From mboxrd@z Thu Jan  1 00:00:00 1970
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Subject: Re: [PATCH v1 0/7] Flow API helpers enhancements
Date: Thu, 12 Oct 2017 14:53:11 +0200
Message-ID: <20171012125311.GS13551@6wind.com>
References: <cover.1507193185.git.adrien.mazarguil@6wind.com>
 <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com>
 <20171006080550.GB3871@6wind.com>
 <a4cdbd84-ec50-532f-ff4a-5f5985d46410@intel.com>
 <20171011095754.GH13551@6wind.com>
 <bec6cf48-89f2-4929-6065-675c03257375@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>, dev@dpdk.org
To: Ferruh Yigit <ferruh.yigit@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42])
 by dpdk.org (Postfix) with ESMTP id 4482F1B2CE
 for <dev@dpdk.org>; Thu, 12 Oct 2017 14:53:23 +0200 (CEST)
Received: by mail-wm0-f42.google.com with SMTP id q124so12945410wmb.0
 for <dev@dpdk.org>; Thu, 12 Oct 2017 05:53:23 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <bec6cf48-89f2-4929-6065-675c03257375@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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