patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Bing Zhao <bingz@nvidia.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 17:43:25 +0000
Message-ID: <92bd14b5-a1a3-dc43-b797-08ed02e6da2a@intel.com> (raw)
In-Reply-To: <CY4PR1201MB0072EF7DAE1B24652B116FF0D0ED0@CY4PR1201MB0072.namprd12.prod.outlook.com>

On 11/6/2020 2:20 PM, Bing Zhao wrote:
> 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?
> 

As far as I know checkpatch is not complaining for the long lines of the string, 
even it does I am OK to ignore it.

  reply	other threads:[~2020-11-06 17:43 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
2020-11-06 17:43     ` Ferruh Yigit [this message]
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=92bd14b5-a1a3-dc43-b797-08ed02e6da2a@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=bingz@nvidia.com \
    --cc=dev@dpdk.org \
    --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