Re: [dpdk-dev] [PATCH 4/5] mlx5: add support for flow director

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Feb 18, 2016 at 05:10:16PM +0100, Adrien Mazarguil wrote:
> Hi Bruce,
> 
> On Wed, Feb 17, 2016 at 05:13:44PM +0000, Bruce Richardson wrote:
> > On Fri, Jan 29, 2016 at 11:32:01AM +0100, Adrien Mazarguil wrote:
> > > From: Yaacov Hazan <yaacovh@mellanox.com>
> > > 
> > > Add support for flow director filters (RTE_FDIR_MODE_PERFECT and
> > > RTE_FDIR_MODE_PERFECT_MAC_VLAN modes).
> > > 
> > > This feature requires MLNX_OFED 3.2.
> > > 
> > > Signed-off-by: Yaacov Hazan <yaacovh@mellanox.com>
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > ---
> > Hi Adrien, Yaacov,
> > 
> > this patch raises a lot of warnings (17) with checkpatch. Can you perhaps look
> > to see if this number can be reduced.
> 
> We actually did it before submitting that patch, there is indeed a bunch of
> remaining warnings that have been left on purpose. Not sure if we have the
> same configuration for checkpatch, but they should fall into the following
> categories:
> 
> - "WARNING: return of an errno should typically be negative" - All return
>   values are documented in the code. Since this PMD uses syscalls in its
>   control path, it uses positive errno values internally for
>   consistency. Public callback functions however return negative error
>   values.
> 
> - "WARNING: line over 80 characters" - Well, although I'm a big fan of the
>   80 characters limit, breaking those would have made the code harder to
>   read.
> 
> - "WARNING: Missing a blank line after declarations" - It's actually a
>   declaration through a macro, there is no missing blank line.
> 
> - "WARNING: networking block comments don't use an empty /* line" - Not sure
>   if we really care? I don't particularly mind.
> 
> - "CHECK: Comparison to NULL could be written "!" - I do not mind either,
>   writing the full check seems clearer to me.
> 
> - "CHECK: Unnecessary parentheses around fdir_info->mask" - Looks like a
>   valid, although minor error.
> 
> Please tell me which of these still need to be fixed.
> 
> -- 
Hi Adrien,

sorry for the delayed reply, I was out for a couple of days.

As none of the above are errors, I'm not going to mandate that they be fixed
before I merge in the patch, so long as you as maintainer are happy with them.

My request mainly came about because of the sheer number of warnings that were
being flagged. To keep the codebase clean requires constant discipline, so I
don't like seeing patches where 17 warnings are flagged. I was hoping since
a new rev of the set was likely anyway that some steps could be taken to reduce
that number.

Thomas, any thoughts here, since I'm still "learning the ropes" as committer. 
Do you have any rules-of-thumb or guidelines as regards checkpatch warnings? The
contributor guide only seems to cover running checkpatch, not anything about
what to do with the output.

Regards,
/Bruce