Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Friday, November 6, 2020 7:35 PM
> To: Bing Zhao <bi...@nvidia.com>; Slava Ovsiienko
> <viachesl...@nvidia.com>; Matan Azrad <ma...@nvidia.com>
> Cc: dev@dpdk.org; Ori Kam <or...@nvidia.com>; Raslan Darawsheh
> <rasl...@nvidia.com>; sta...@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix eCPRI previous
> layer checking
>
> External email: Use caution opening links or attachments
>
>
> On 11/3/2020 5:42 AM, Bing Zhao wrote:
> > Based on the specification, eCPRI can only follow ETH (VLAN) layer
> or
> > UDP layer. When creating a flow with eCPRI item, this should be
> > checked and invalid layout of the layers should be rejected.
> >
> > Fixes: c7eca23657b7 ("net/mlx5: add flow validation of eCPRI
> header")
> >
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bi...@nvidia.com>
> > Acked-by: Viacheslav Ovsiienko <viachesl...@nvidia.com>
> > ---
> > drivers/net/mlx5/mlx5_flow.c | 29 ++++++++++++++++++-----------
> > 1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index a6e60af..11dba3b 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -2896,17 +2896,23 @@ struct mlx5_flow_tunnel_info {
> > MLX5_FLOW_LAYER_OUTER_VLAN);
> > struct rte_flow_item_ecpri mask_lo;
> >
> > + if (!(last_item & outer_l2_vlan) &&
> > + last_item != MLX5_FLOW_LAYER_OUTER_L4_UDP)
> > + return rte_flow_error_set(error, EINVAL,
> > + RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > + "eCPRI can only follow
> L2/VLAN layer"
> > + " or UDP layer.");
> > if ((last_item & outer_l2_vlan) && ether_type &&
> > ether_type != RTE_ETHER_TYPE_ECPRI)
> > return rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > - "eCPRI cannot follow
> L2/VLAN layer "
> > - "which ether type is not
> 0xAEFE.");
> > + "eCPRI cannot follow
> L2/VLAN layer"
> > + " which ether type is not
> > + 0xAEFE.");
> > if (item_flags & MLX5_FLOW_LAYER_TUNNEL)
> > return rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > - "eCPRI with tunnel is not
> supported "
> > - "right now.");
> > + "eCPRI with tunnel is not
> supported"
> > + " right now.");
>
> Why these changes done, it only moves space from end of first line
> to beginning of the second line?
Yes, because when I am doing the fix. I found this log part is different from
others in the same file and just want to be consistent.
>
> Overall I think no need to break the log strings, keeping them
> intact helps users search the error message in the code.
> I assume the break is because of the 80 chars limit but for log
> strings we don't have that limit, unless it is too long (lets say
> 120 chars as thumb of rule, there is no official convention) I think
> better to not break.
Good point, in the past when I was searching some logs and I failed due to the
long log line break.
>
> What do you think remove the whitespace changes out of this commit
> and make another patch to merge the log strings?
Yes I can and will send v2 of this.
Or should I keep the log in a single line @Slava Ovsiienko, what do you think?
Any comments?
I remember in the past, my "checkpatch.pl" will report warning against this.
Could we ignore this?
BR. Bing
