patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Bing Zhao <bingz@nvidia.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Slava Ovsiienko <viacheslavo@nvidia.com>,
	Matan Azrad <matan@nvidia.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Ori Kam <orika@nvidia.com>,
	"Raslan Darawsheh" <rasland@nvidia.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix eCPRI previous layer checking
Date: Fri, 6 Nov 2020 14:20:12 +0000
Message-ID: <CY4PR1201MB0072EF7DAE1B24652B116FF0D0ED0@CY4PR1201MB0072.namprd12.prod.outlook.com> (raw)
In-Reply-To: <ed6af423-88a1-a6bd-21f2-13aa893317b3@intel.com>

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, November 6, 2020 7:35 PM
> To: Bing Zhao <bingz@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; stable@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: stable@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > Acked-by: Viacheslav Ovsiienko <viacheslavo@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

  reply	other threads:[~2020-11-06 14:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-03  5:42 Bing Zhao
2020-11-05 15:02 ` Raslan Darawsheh
2020-11-06 11:34 ` Ferruh Yigit
2020-11-06 14:20   ` Bing Zhao [this message]
2020-11-06 17:43     ` Ferruh Yigit
2020-11-11  9:28 ` [dpdk-stable] [PATCH v2] " Bing Zhao
2020-11-13 18:08   ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY4PR1201MB0072EF7DAE1B24652B116FF0D0ED0@CY4PR1201MB0072.namprd12.prod.outlook.com \
    --to=bingz@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=matan@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.com \
    --cc=stable@dpdk.org \
    --cc=viacheslavo@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git