[dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ori Kam]

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,
+	};
 	uint8_t *header;
 	int ret;
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ferruh Yigit]

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;
>  
> 

Re: [dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ori Kam]

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;
> >
> >

Re: [dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ferruh Yigit]

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;
>>>
>>>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ferruh Yigit]

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;
>>>>
>>>>
>>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ori Kam]

> -----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;
> >>>>
> >>>>
> >>
> >

Re: [dpdk-dev] [PATCH] app/testpmd: fix uninitialized members of mpls

[Ferruh Yigit]

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.