staging/dgap/dgap.c:981: bad if test ?

staging/dgap/dgap.c:981: bad if test ?

[David Binderman]

Hello there,

[linux-3.19-rc5/drivers/staging/dgap/dgap.c:981]: (warning) Logical disjunction always evaluates to true: conc_type != 65 || conc_type != 66.

Source code is

            if (conc_type == 0 || conc_type != CX ||
                conc_type != EPC) {

Suggest code rework.

Regards

David Binderman

 		 	   		  
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging/dgap/dgap.c:981: bad if test ?

[Dan Carpenter]

On Thu, Jan 22, 2015 at 10:05:53AM +0000, David Binderman wrote:
> Hello there,
> 
> [linux-3.19-rc5/drivers/staging/dgap/dgap.c:981]: (warning) Logical disjunction always evaluates to true: conc_type != 65 || conc_type != 66.
> 
> Source code is
> 
>             if (conc_type == 0 || conc_type != CX ||
>                 conc_type != EPC) {
> 
> Suggest code rework.

Thanks.  This bug was introduced in:

commit f6aa0164cd3b1c5192e87f5651ec382c3bc3abac
Author: Daeseok Youn <daeseok.youn@gmail.com>
Date:   Sat Aug 9 14:38:41 2014 +0900

    staging: dgap: Simplify to set a concentrator type

    It is same manner with setting a board type.
    For example of config file for concentrator,
        "conc ccon" or
        "conc epcon"

    After allocating a type of "CNODE" then set a type of concentrator.
    So remove cases in swith statement, just get a token from string
    and set to "conc.type". And also it doesn't need to "conc.v_type".

    Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Let's CC Daeseok.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: staging/dgap/dgap.c:981: bad if test ?

[David Binderman]

Hello there,

Thanks for the confirmation. More of the same in the same file:

[linux-3.19-rc5/drivers/staging/dgap/dgap.c:1022]: (warning) Logical disjunction always evaluates to true: module_type != 68 || module_type != 73.

Source code is

            if (module_type == 0 || module_type != PORTS ||
                module_type != MODEM) {

I used the static analyser cppcheck to find these two bugs, but
you might be able to find similar problems by using gcc compiler
flag -Wlogical-op.

Regards

David Binderman


----------------------------------------
> Date: Thu, 22 Jan 2015 13:14:22 +0300
> From: dan.carpenter@oracle.com
> To: dcb314@hotmail.com; daeseok.youn@gmail.com
> CC: driverdev-devel@linuxdriverproject.org
> Subject: Re: staging/dgap/dgap.c:981: bad if test ?
>
>
> On Thu, Jan 22, 2015 at 10:05:53AM +0000, David Binderman wrote:
>> Hello there,
>>
>> [linux-3.19-rc5/drivers/staging/dgap/dgap.c:981]: (warning) Logical disjunction always evaluates to true: conc_type != 65 || conc_type != 66.
>>
>> Source code is
>>
>>             if (conc_type == 0 || conc_type != CX ||
>>                 conc_type != EPC) {
>>
>> Suggest code rework.
>
> Thanks. This bug was introduced in:
>
> commit f6aa0164cd3b1c5192e87f5651ec382c3bc3abac
> Author: Daeseok Youn <daeseok.youn@gmail.com>
> Date: Sat Aug 9 14:38:41 2014 +0900
>
> staging: dgap: Simplify to set a concentrator type
>
> It is same manner with setting a board type.
> For example of config file for concentrator,
> "conc ccon" or
> "conc epcon"
>
> After allocating a type of "CNODE" then set a type of concentrator.
> So remove cases in swith statement, just get a token from string
> and set to "conc.type". And also it doesn't need to "conc.v_type".
>
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Let's CC Daeseok.
>
> regards,
> dan carpenter
>
 		 	   		  
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging/dgap/dgap.c:981: bad if test ?

[Dan Carpenter]

On Thu, Jan 22, 2015 at 10:20:44AM +0000, David Binderman wrote:
> I used the static analyser cppcheck to find these two bugs, but
> you might be able to find similar problems by using gcc compiler
> flag -Wlogical-op.

I turned it on, but GCC 4.7.2 doesn't find anything for me.

It complains about:
	if (x != 1 || x != 2)

but it doesn't complain about:
	if (x == 0 || x != 1 || x != 2) 

Smatch is similar to GCC in this regard...

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: staging/dgap/dgap.c:981: bad if test ?

[David Binderman]

Hello there,

----------------------------------------
> On Thu, Jan 22, 2015 at 10:20:44AM +0000, David Binderman wrote:
>> I used the static analyser cppcheck to find these two bugs, but
>> you might be able to find similar problems by using gcc compiler
>> flag -Wlogical-op.
>
> I turned it on, but GCC 4.7.2 doesn't find anything for me.

Surprising. 

I note that gcc 4.7.2 is dated Sep 2012 and the current release of gcc is 4.9.2, dated Oct 2014.
I am not sure upgrading gcc will help.

> Smatch is similar to GCC in this regard...

cppcheck is available from sourceforge.

Regards

David Binderman
 		 	   		  
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging/dgap/dgap.c:981: bad if test ?

[DaeSeok Youn]

Hi,

2015-01-22 19:14 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
>
> On Thu, Jan 22, 2015 at 10:05:53AM +0000, David Binderman wrote:
>> Hello there,
>>
>> [linux-3.19-rc5/drivers/staging/dgap/dgap.c:981]: (warning) Logical disjunction always evaluates to true: conc_type != 65 || conc_type != 66.
>>
>> Source code is
>>
>>             if (conc_type == 0 || conc_type != CX ||
>>                 conc_type != EPC) {
>>
>> Suggest code rework.
>
> Thanks.  This bug was introduced in:
Yes, right. I will fix this bug.

The configuration file has a type of "concentrator" and that type has
two different modules that are "ccon" and "epc".
If configuration file has another type for "concentrator", returns
"-1" but it always return an error.

Thanks for reporting.

regards,
Daeseok Youn.

>
> commit f6aa0164cd3b1c5192e87f5651ec382c3bc3abac
> Author: Daeseok Youn <daeseok.youn@gmail.com>
> Date:   Sat Aug 9 14:38:41 2014 +0900
>
>     staging: dgap: Simplify to set a concentrator type
>
>     It is same manner with setting a board type.
>     For example of config file for concentrator,
>         "conc ccon" or
>         "conc epcon"
>
>     After allocating a type of "CNODE" then set a type of concentrator.
>     So remove cases in swith statement, just get a token from string
>     and set to "conc.type". And also it doesn't need to "conc.v_type".
>
>     Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Let's CC Daeseok.
>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel