[PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The previous fix removed the equal to zero comparisons by the strcmps and
now the function always returns true. Fix this by adding in the missing
logical negation operators.

Detected by CoverityScan, CID#1452267 ("Constant expression result")

Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/staging/fsl-mc/bus/fsl-mc-allocator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c b/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
index b37a6f48225f..8b5f2d0cdf06 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
@@ -16,9 +16,9 @@
 
 static bool __must_check fsl_mc_is_allocatable(const char *obj_type)
 {
-	return strcmp(obj_type, "dpbp") ||
-	       strcmp(obj_type, "dpmcp") ||
-	       strcmp(obj_type, "dpcon");
+	return !strcmp(obj_type, "dpbp") ||
+	       !strcmp(obj_type, "dpmcp") ||
+	       !strcmp(obj_type, "dpcon");
 }
 
 /**
-- 
2.11.0

[PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The previous fix removed the equal to zero comparisons by the strcmps and
now the function always returns true. Fix this by adding in the missing
logical negation operators.

Detected by CoverityScan, CID#1452267 ("Constant expression result")

Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/staging/fsl-mc/bus/fsl-mc-allocator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c b/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
index b37a6f48225f..8b5f2d0cdf06 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-allocator.c
@@ -16,9 +16,9 @@
 
 static bool __must_check fsl_mc_is_allocatable(const char *obj_type)
 {
-	return strcmp(obj_type, "dpbp") ||
-	       strcmp(obj_type, "dpmcp") ||
-	       strcmp(obj_type, "dpcon");
+	return !strcmp(obj_type, "dpbp") ||
+	       !strcmp(obj_type, "dpmcp") ||
+	       !strcmp(obj_type, "dpcon");
 }
 
 /**
-- 
2.11.0

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Dan Carpenter]

On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The previous fix removed the equal to zero comparisons by the strcmps and
> now the function always returns true. Fix this by adding in the missing
> logical negation operators.
> 
> Detected by CoverityScan, CID#1452267 ("Constant expression result")
> 
> Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")

Ugh...  I did review the original patch at all.  Sorry.  It's better to
use "= 0" because it's idiomatic.

	strcmp(foo, bar) = 0 means foo = bar
	strcmp(foo, bar) != 0 means foo != bar
	strcmp(foo, bar) < 0 means foo < bar alphabetically.

It's way more readable.  strcmp() bugs are fairly common when people
don't use = 0 and != 0.  There are other places where != 0 hurts
readability, such as checking for errors:

	if (frob() != 0) {

In this case, frob() is returning negative error codes, but it's not
really returning the number zero, it's returning "success".  So it
should be:

	ret = frob();
	if (ret) {

Comparing against NULL really doesn't add anything either.  But if
you're talking about the number zero then you should use = 0.

	if (len = 0)
		return 0;

regards,
dan carpenter

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Dan Carpenter]

On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The previous fix removed the equal to zero comparisons by the strcmps and
> now the function always returns true. Fix this by adding in the missing
> logical negation operators.
> 
> Detected by CoverityScan, CID#1452267 ("Constant expression result")
> 
> Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")

Ugh...  I did review the original patch at all.  Sorry.  It's better to
use "== 0" because it's idiomatic.

	strcmp(foo, bar) == 0 means foo == bar
	strcmp(foo, bar) != 0 means foo != bar
	strcmp(foo, bar) < 0 means foo < bar alphabetically.

It's way more readable.  strcmp() bugs are fairly common when people
don't use == 0 and != 0.  There are other places where != 0 hurts
readability, such as checking for errors:

	if (frob() != 0) {

In this case, frob() is returning negative error codes, but it's not
really returning the number zero, it's returning "success".  So it
should be:

	ret = frob();
	if (ret) {

Comparing against NULL really doesn't add anything either.  But if
you're talking about the number zero then you should use == 0.

	if (len == 0)
		return 0;

regards,
dan carpenter

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Dan Carpenter]

On Wed, Aug 16, 2017 at 03:06:54PM +0300, Dan Carpenter wrote:
> On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The previous fix removed the equal to zero comparisons by the strcmps and
> > now the function always returns true. Fix this by adding in the missing
> > logical negation operators.
> > 
> > Detected by CoverityScan, CID#1452267 ("Constant expression result")
> > 
> > Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
> 
> Ugh...  I did review the original patch at all.  Sorry.

s/did/did not/

regards,
dan carpenter

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Dan Carpenter]

On Wed, Aug 16, 2017 at 03:06:54PM +0300, Dan Carpenter wrote:
> On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The previous fix removed the equal to zero comparisons by the strcmps and
> > now the function always returns true. Fix this by adding in the missing
> > logical negation operators.
> > 
> > Detected by CoverityScan, CID#1452267 ("Constant expression result")
> > 
> > Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
> 
> Ugh...  I did review the original patch at all.  Sorry.

s/did/did not/

regards,
dan carpenter

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Laurentiu Tudor]

On 08/16/2017 03:06 PM, Dan Carpenter wrote:
> On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The previous fix removed the equal to zero comparisons by the strcmps and
>> now the function always returns true. Fix this by adding in the missing
>> logical negation operators.
>>
>> Detected by CoverityScan, CID#1452267 ("Constant expression result")
>>
>> Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")

Thanks Colin (and Coverity) for catching this!

> Ugh...  I did review the original patch at all.  Sorry.

As a side note, funny how i got the patch description right but not the 
actual patch. :-)

> It's better to use "== 0" because it's idiomatic.

Agree, plus this approach would be consistent with the rest of the 
driver (except one place in drivers/staging/fsl-mc/bus/dprc-driver.c +32)

---
Best Regards, Laurentiu

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Colin Ian King]

On 16/08/17 14:37, Laurentiu Tudor wrote:
> On 08/16/2017 03:06 PM, Dan Carpenter wrote:
>> On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The previous fix removed the equal to zero comparisons by the strcmps and
>>> now the function always returns true. Fix this by adding in the missing
>>> logical negation operators.
>>>
>>> Detected by CoverityScan, CID#1452267 ("Constant expression result")
>>>
>>> Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
> 
> Thanks Colin (and Coverity) for catching this!
> 
>> Ugh...  I did review the original patch at all.  Sorry.
> 
> As a side note, funny how i got the patch description right but not the 
> actual patch. :-)
> 
>> It's better to use "= 0" because it's idiomatic.
> 
> Agree, plus this approach would be consistent with the rest of the 
> driver (except one place in drivers/staging/fsl-mc/bus/dprc-driver.c +32)

OK, I'll send a revert sometime today as that seems the sane solution.

Colin
> 
> ---
> Best Regards, Laurentiu--
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] staging: fsl-mc: fix fsl_mc_is_allocatable strcmps

[Colin Ian King]

On 16/08/17 14:37, Laurentiu Tudor wrote:
> On 08/16/2017 03:06 PM, Dan Carpenter wrote:
>> On Wed, Aug 16, 2017 at 12:44:51PM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The previous fix removed the equal to zero comparisons by the strcmps and
>>> now the function always returns true. Fix this by adding in the missing
>>> logical negation operators.
>>>
>>> Detected by CoverityScan, CID#1452267 ("Constant expression result")
>>>
>>> Fixes: b93ad9a067e1 ("staging: fsl-mc: be consistent when checking strcmp() return")
> 
> Thanks Colin (and Coverity) for catching this!
> 
>> Ugh...  I did review the original patch at all.  Sorry.
> 
> As a side note, funny how i got the patch description right but not the 
> actual patch. :-)
> 
>> It's better to use "== 0" because it's idiomatic.
> 
> Agree, plus this approach would be consistent with the rest of the 
> driver (except one place in drivers/staging/fsl-mc/bus/dprc-driver.c +32)

OK, I'll send a revert sometime today as that seems the sane solution.

Colin
> 
> ---
> Best Regards, Laurentiu--
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>