diff mbox series

app/testpmd: fix uninitialized members of mpls

Message ID 1580403575-142634-1-git-send-email-orika@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series app/testpmd: fix uninitialized members of mpls |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation fail Compilation issues

Commit Message

Ori Kam Jan. 30, 2020, 4:59 p.m. UTC 
  Some of the memebers of the mpls struct are not initialized.
this commit init the uninitialized members.

Coverity issue: 325735
Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
Cc: stable@dpdk.org

Signed-off-by: Ori Kam <orika@mellanox.com>
---
 app/test-pmd/cmdline_flow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Jan. 31, 2020, 11:07 a.m. UTC | #1 
On 1/30/2020 4:59 PM, Ori Kam wrote:
> Some of the memebers of the mpls struct are not initialized.
> this commit init the uninitialized members.
> 
> Coverity issue: 325735
> Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
>  app/test-pmd/cmdline_flow.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index e99e24c..c2cc4c5 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -4576,7 +4576,9 @@ static int comp_set_raw_index(struct context *, const struct token *,
>  	struct rte_flow_item_gre gre = {
>  		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
>  	};
> -	struct rte_flow_item_mpls mpls;
> +	struct rte_flow_item_mpls mpls = {
> +		.ttl = 0,
> +	};

why not use "= {0};", the end result will be same, struct will be all zeroed out
in both case, "= {0};" makes the intention more obvious I think.

>  	uint8_t *header;
>  	int ret;
>  
>
Ori Kam Feb. 2, 2020, 8:23 a.m. UTC | #2 
Hi Ferruh,

PSB,
Thanks,
Ori

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, January 31, 2020 1:08 PM
> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix uninitialized members of mpls
> 
> On 1/30/2020 4:59 PM, Ori Kam wrote:
> > Some of the memebers of the mpls struct are not initialized.
> > this commit init the uninitialized members.
> >
> > Coverity issue: 325735
> > Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> >  app/test-pmd/cmdline_flow.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index e99e24c..c2cc4c5 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -4576,7 +4576,9 @@ static int comp_set_raw_index(struct context *,
> const struct token *,
> >  	struct rte_flow_item_gre gre = {
> >  		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
> >  	};
> > -	struct rte_flow_item_mpls mpls;
> > +	struct rte_flow_item_mpls mpls = {
> > +		.ttl = 0,
> > +	};
> 
> why not use "= {0};", the end result will be same, struct will be all zeroed out
> in both case, "= {0};" makes the intention more obvious I think.
> 

On some compilers this kind of  initialization result in an error:
error: missing braces around initializer
it looks like a known issue of GCC
So I just prefer to init the relevant fields.

> >  	uint8_t *header;
> >  	int ret;
> >
> >
Ferruh Yigit Feb. 3, 2020, 10:21 a.m. UTC | #3 
On 2/2/2020 8:23 AM, Ori Kam wrote:
> Hi Ferruh,
> 
> PSB,
> Thanks,
> Ori
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Friday, January 31, 2020 1:08 PM
>> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
>> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
>> <bernard.iremonger@intel.com>
>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>;
>> stable@dpdk.org
>> Subject: Re: [PATCH] app/testpmd: fix uninitialized members of mpls
>>
>> On 1/30/2020 4:59 PM, Ori Kam wrote:
>>> Some of the memebers of the mpls struct are not initialized.
>>> this commit init the uninitialized members.
>>>
>>> Coverity issue: 325735
>>> Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>> ---
>>>  app/test-pmd/cmdline_flow.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index e99e24c..c2cc4c5 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -4576,7 +4576,9 @@ static int comp_set_raw_index(struct context *,
>> const struct token *,
>>>  	struct rte_flow_item_gre gre = {
>>>  		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
>>>  	};
>>> -	struct rte_flow_item_mpls mpls;
>>> +	struct rte_flow_item_mpls mpls = {
>>> +		.ttl = 0,
>>> +	};
>>
>> why not use "= {0};", the end result will be same, struct will be all zeroed out
>> in both case, "= {0};" makes the intention more obvious I think.
>>
> 
> On some compilers this kind of  initialization result in an error:
> error: missing braces around initializer

As far as I remember, that is triggered with old compiler, when you are
initializing array of structs, or first element of the struct is an array (which
is the case for mpls struct), in that case ={{0}} was solving the issue.

Anyway, I got the concern, the patch looks good to me.

> it looks like a known issue of GCC
> So I just prefer to init the relevant fields.
> 
>>>  	uint8_t *header;
>>>  	int ret;
>>>
>>>
>
Ferruh Yigit Feb. 3, 2020, 5:46 p.m. UTC | #4 
On 2/3/2020 10:21 AM, Ferruh Yigit wrote:
> On 2/2/2020 8:23 AM, Ori Kam wrote:
>> Hi Ferruh,
>>
>> PSB,
>> Thanks,
>> Ori
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Friday, January 31, 2020 1:08 PM
>>> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
>>> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
>>> <bernard.iremonger@intel.com>
>>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>;
>>> stable@dpdk.org
>>> Subject: Re: [PATCH] app/testpmd: fix uninitialized members of mpls
>>>
>>> On 1/30/2020 4:59 PM, Ori Kam wrote:
>>>> Some of the memebers of the mpls struct are not initialized.
>>>> this commit init the uninitialized members.
>>>>
>>>> Coverity issue: 325735
>>>> Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>>> ---
>>>>  app/test-pmd/cmdline_flow.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>> index e99e24c..c2cc4c5 100644
>>>> --- a/app/test-pmd/cmdline_flow.c
>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>> @@ -4576,7 +4576,9 @@ static int comp_set_raw_index(struct context *,
>>> const struct token *,
>>>>  	struct rte_flow_item_gre gre = {
>>>>  		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
>>>>  	};
>>>> -	struct rte_flow_item_mpls mpls;
>>>> +	struct rte_flow_item_mpls mpls = {
>>>> +		.ttl = 0,
>>>> +	};
>>>
>>> why not use "= {0};", the end result will be same, struct will be all zeroed out
>>> in both case, "= {0};" makes the intention more obvious I think.
>>>
>>
>> On some compilers this kind of  initialization result in an error:
>> error: missing braces around initializer
> 
> As far as I remember, that is triggered with old compiler, when you are
> initializing array of structs, or first element of the struct is an array (which
> is the case for mpls struct), in that case ={{0}} was solving the issue.
> 
> Anyway, I got the concern, the patch looks good to me.

Also seems "={}" is working in that case, thanks to Stephen, it is cleaner than
"={{0}}", what do you think about using "={}" ?

> 
>> it looks like a known issue of GCC
>> So I just prefer to init the relevant fields.
>>
>>>>  	uint8_t *header;
>>>>  	int ret;
>>>>
>>>>
>>
>
Ori Kam Feb. 4, 2020, 7:50 a.m. UTC | #5 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, February 3, 2020 7:46 PM
> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix uninitialized members of
> mpls
> 
> On 2/3/2020 10:21 AM, Ferruh Yigit wrote:
> > On 2/2/2020 8:23 AM, Ori Kam wrote:
> >> Hi Ferruh,
> >>
> >> PSB,
> >> Thanks,
> >> Ori
> >>
> >>> -----Original Message-----
> >>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> Sent: Friday, January 31, 2020 1:08 PM
> >>> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>;
> >>> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> >>> <bernard.iremonger@intel.com>
> >>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>;
> >>> stable@dpdk.org
> >>> Subject: Re: [PATCH] app/testpmd: fix uninitialized members of mpls
> >>>
> >>> On 1/30/2020 4:59 PM, Ori Kam wrote:
> >>>> Some of the memebers of the mpls struct are not initialized.
> >>>> this commit init the uninitialized members.
> >>>>
> >>>> Coverity issue: 325735
> >>>> Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Ori Kam <orika@mellanox.com>
> >>>> ---
> >>>>  app/test-pmd/cmdline_flow.c | 4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> >>>> index e99e24c..c2cc4c5 100644
> >>>> --- a/app/test-pmd/cmdline_flow.c
> >>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>> @@ -4576,7 +4576,9 @@ static int comp_set_raw_index(struct context *,
> >>> const struct token *,
> >>>>  	struct rte_flow_item_gre gre = {
> >>>>  		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
> >>>>  	};
> >>>> -	struct rte_flow_item_mpls mpls;
> >>>> +	struct rte_flow_item_mpls mpls = {
> >>>> +		.ttl = 0,
> >>>> +	};
> >>>
> >>> why not use "= {0};", the end result will be same, struct will be all zeroed
> out
> >>> in both case, "= {0};" makes the intention more obvious I think.
> >>>
> >>
> >> On some compilers this kind of  initialization result in an error:
> >> error: missing braces around initializer
> >
> > As far as I remember, that is triggered with old compiler, when you are
> > initializing array of structs, or first element of the struct is an array (which
> > is the case for mpls struct), in that case ={{0}} was solving the issue.
> >
> > Anyway, I got the concern, the patch looks good to me.
> 
> Also seems "={}" is working in that case, thanks to Stephen, it is cleaner than
> "={{0}}", what do you think about using "={}" ?
> 
Thanks for the idea, but it still causes issue,
"error: missing initializer for field 'label_tc_s' of 'struct rte_flow_item_mpls' [-Werror=missing-field-initializers]
 struct rte_flow_item_mpls mpls = {};"
Just for reference  I'm using gcc 4.8.5 (Red Hat 4.8.5-4)
I know it is very old, but some of our customers are using it.
So if you don't mind I will keep my code as is.

> >
> >> it looks like a known issue of GCC
> >> So I just prefer to init the relevant fields.
> >>
> >>>>  	uint8_t *header;
> >>>>  	int ret;
> >>>>
> >>>>
> >>
> >
Ferruh Yigit Feb. 4, 2020, 11:49 a.m. UTC | #6 
On 1/30/2020 4:59 PM, Ori Kam wrote:
> Some of the memebers of the mpls struct are not initialized.
> this commit init the uninitialized members.
> 
> Coverity issue: 325735
> Fixes: 3e77031be855 ("app/testpmd: add MPLSoGRE encapsulation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index e99e24c..c2cc4c5 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4576,7 +4576,9 @@  static int comp_set_raw_index(struct context *, const struct token *,
 	struct rte_flow_item_gre gre = {
 		.protocol = rte_cpu_to_be_16(ETHER_TYPE_MPLS_UNICAST),
 	};
-	struct rte_flow_item_mpls mpls;
+	struct rte_flow_item_mpls mpls = {
+		.ttl = 0,
+	};
 	uint8_t *header;
 	int ret;