x86,boot: standardize strcmp()

PATCH Applied Arjun Sreedharan 6 participants 11 messages Mar 16, 2015 – Mar 23, 2015

Subsystems (2)

Arm (32-bit) x86

lore

Share & tools

lore

Arjun Sreedharan

author

2015-03-16 15:38 UTC #0 lore raw

PATCH 1/1 x86,boot: standardize strcmp()

strcmp() is always expected to return 0 when args are same, <0 when arg1 is lesser and >0 otherwise. Previously strcmp("a","b") returned 1. Now it gives -1.

Signed-off-by Arjun Sreedharan <[email protected]>

---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/boot/string.c +1 -1

index 493f3fd..318b846 100644
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;

Signature · 1 line

1.8.1.msysgit.1

Borislav Petkov

maintainer Applied

2015-03-16 18:18 UTC #1 lore raw

Quoted 15 lines · Arjun Sreedharan

strcmp() is always expected to return 0 when args are same, <0 when arg1 is lesser and >0 otherwise. Previously strcmp("a","b") returned 1. Now it gives -1.
Signed-off-by Arjun Sreedharan <[email protected]>
---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/boot/string.c +0 -0
index 493f3fd..318b846 100644

arch/x86/boot/string.c int strcmp(const char *str1, const char *str2) · line 36

Quoted 8 lines

arch/x86/boot/string.c +1 -1

@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;

Applied, thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Ingo Molnar

2015-03-17 07:46 UTC #2 lore raw

In reply to Arjun Sreedharan

Quoted 3 lines · Arjun Sreedharan

strcmp() is always expected to return 0 when args are same, <0 when arg1 is lesser and >0 otherwise. Previously strcmp("a","b") returned 1. Now it gives -1.

I'd also add the following to the changelog:

Until now this bug never triggered, because all uses for strcmp() in the boot code tested for nonzero:

triton:~/tip> git grep strcmp arch/x86/boot/ arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);

 arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot code, it might have led to (not so subtle) bugs.

Thanks,

Ingo

Arjun Sreedharan

author

2015-03-17 14:13 UTC #3 lore raw

Quoted 11 lines · Ingo Molnar

Quoted 3 lines · Arjun Sreedharan
strcmp() is always expected to return 0 when args are same, <0 when arg1 is lesser and >0 otherwise. Previously strcmp("a","b") returned 1. Now it gives -1.
I'd also add the following to the changelog:
Until now this bug never triggered, because all uses for strcmp() in the boot code tested for nonzero:

On a related note, IMO strcmp() should return {-1,0,1} since many programmers just expect this behavior. just my opinion.

Arjun Sreedharan

Quoted 13 lines

triton:~/tip> git grep strcmp arch/x86/boot/ arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
 arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
 arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))
should in the future strcmp() be used in a comparative way in the boot code, it might have led to (not so subtle) bugs.
Thanks,
Ingo

Borislav Petkov

maintainer

2015-03-17 14:29 UTC #9 lore raw

In reply to Ingo Molnar

Quoted 1 line · Ingo Molnar

I'd also add the following to the changelog:

Added:

---
From: Arjun Sreedharan <[email protected]>
Date: Mon, 16 Mar 2015 21:07:47 +0530
Subject: [PATCH] x86/boot: Standardize strcmp()

strcmp() is always expected to return 0 when arguments are equal, negative when its first argument @str1 is less than its second argument @str2 and a positive value otherwise. Previously strcmp("a", "b") returned 1. Now it gives -1, as it is supposed to.

Until now this bug never triggered, because all uses for strcmp() in the boot code tested for nonzero:

  triton:~/tip> git grep strcmp arch/x86/boot/
  arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
  arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot code, it might have led to (not so subtle) bugs.

Signed-off-by Arjun Sreedharan <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/boot/string.c +1 -1

index 493f3fd9f139..318b8465d302 100644
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;

Signature · 8 lines

2.3.3

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Bernd Petrovitsch

2015-03-18 02:02 UTC #4 lore raw

In reply to Arjun Sreedharan

On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
[...]

Quoted 2 lines

On a related note, IMO strcmp() should return {-1,0,1} since many programmers just expect this behavior. just my opinion.

-ENOPATCH.

MfG,
Bernd

Signature · 4 lines

"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

Arjun Sreedharan

author

2015-03-18 18:10 UTC #5 lore raw

Quoted 6 lines · Bernd Petrovitsch

On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
[...]
Quoted 2 lines
On a related note, IMO strcmp() should return {-1,0,1} since many programmers just expect this behavior. just my opinion.
-ENOPATCH.

Here's a patch

-- >8 --

Subject: [PATCH] arm,x86: limit strcmp() rc to {-1,0,1}

Signed-off-by Arjun Sreedharan <[email protected]>

---
 arch/arm/boot/compressed/string.c | 2 +-
 arch/x86/boot/string.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/arch/arm/boot/compressed/string.c
index 36e53ef..e48df86 100644

arch/arm/boot/compressed/string.c +1 -1

@@ -88,7 +88,7 @@ int strcmp(const char *cs, const char *ct)
  c2 = *ct++;
  res = c1 - c2;
  if (res)
- break;
+ return res < 0 ? -1 : 1;
  } while (c1);
  return res;
 }

arch/x86/boot/string.c +1 -1

index 318b846..6eb333e 100644
@@ -32,7 +32,7 @@ int strcmp(const char *str1, const char *str2)
  while (*s1 || *s2) {
  delta = *s1 - *s2;
  if (delta)
- return delta;
+ return delta < 0 ? -1 : 1;
  s1++;
  s2++;
  }

Quoted 9 lines

MfG,
Bernd
--
"I dislike type abstraction if it has no real reason. And saving on typing is not a good reason - if your typing speed is the main issue when you're coding, you're doing something seriously wrong."
- Linus Torvalds

Arjun Sreedharan

author

2015-03-18 18:37 UTC #6 lore raw

PATCH 1/1 x86,boot: standardize strcmp()

In reply to Bernd Petrovitsch

Sorry for the previous messed up email.
Here's a patch.

-- >8 --

Subject: [PATCH] arm,x86: limit strcmp() rc to {-1,0,1}

Signed-off-by Arjun Sreedharan <[email protected]>

---
 arch/arm/boot/compressed/string.c | 2 +-
 arch/x86/boot/string.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/boot/compressed/string.c +1 -1

index 36e53ef..e48df86 100644
@@ -88,7 +88,7 @@ int strcmp(const char *cs, const char *ct)
 		c2 = *ct++;
 		res = c1 - c2;
 		if (res)
-			break;
+			return res < 0 ? -1 : 1;
 	} while (c1);
 	return res;
 }

arch/x86/boot/string.c +1 -1

index 318b846..6eb333e 100644
@@ -32,7 +32,7 @@ int strcmp(const char *str1, const char *str2)
 	while (*s1 || *s2) {
 		delta = *s1 - *s2;
 		if (delta)
-			return delta;
+			return delta < 0 ? -1 : 1;
 		s1++;
 		s2++;
 	}

H. Peter Anvin

reviewer

2015-03-19 17:34 UTC #7 lore raw

In reply to Arjun Sreedharan

Quoted 4 lines · Arjun Sreedharan

On a related note, IMO strcmp() should return {-1,0,1} since many programmers just expect this behavior. just my opinion.

I would challenge that assumption, *especially* in the context of kernel programming. Let's not waste time on that crap.

-hpa

Bernd Petrovitsch

2015-03-20 11:43 UTC #8 lore raw

Quoted 3 lines · H. Peter Anvin

Quoted 2 lines · Arjun Sreedharan
On a related note, IMO strcmp() should return {-1,0,1} since many programmers just expect this behavior. just my opinion.

One doesn't change an API just for a claimed expection for an unprooved number of cases.

Quoted 2 lines

I would challenge that assumption, *especially* in the context of kernel programming. Let's not waste time on that crap.

Even if the assumption is correct (which I'm not implying - quite the opposite), than these programmers are not well educated enough and - thus;-) - write buggy code. They also fail to strive for mot possible robustness.

BTW POSIX' strcmp() description on
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strcmp.html
als states "> 0, == 0 or < 0" (and ISO-C seem to also see it that way).

Kind regards,
Bernd

Signature · 4 lines

"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
    - Linus Torvalds

tip-bot for Arjun Sreedharan

merge bot

2015-03-23 12:23 UTC #10 lore raw

[tip:x86/boot] x86/boot: Standardize strcmp()

In reply to Arjun Sreedharan

x86/boot: Standardize strcmp()

Commit-ID:  1c1d046be692493d00a4831d4fbc266745008e09
Gitweb:     http://git.kernel.org/tip/1c1d046be692493d00a4831d4fbc266745008e09
Author:     Arjun Sreedharan <[email protected]>
AuthorDate: Mon, 16 Mar 2015 21:07:47 +0530
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 10:24:12 +0100

x86/boot: Standardize strcmp()

Until now this bug never triggered, because all uses for strcmp() in the boot code tested for nonzero:

  triton:~/tip> git grep strcmp arch/x86/boot/
  arch/x86/boot/boot.h:int strcmp(const char *str1, const char *str2);
  arch/x86/boot/edd.c:            if (!strcmp(eddarg, "skipmbr") || !strcmp(eddarg, "skip")) {
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "off"))
  arch/x86/boot/edd.c:            else if (!strcmp(eddarg, "on"))

should in the future strcmp() be used in a comparative way in the boot code, it might have led to (not so subtle) bugs.

Signed-off-by Arjun Sreedharan <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/boot/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/boot/string.c +1 -1

index 493f3fd..318b846 100644
@@ -30,7 +30,7 @@ int strcmp(const char *str1, const char *str2)
 	int delta = 0;
 
 	while (*s1 || *s2) {
-		delta = *s2 - *s1;
+		delta = *s1 - *s2;
 		if (delta)
 			return delta;
 		s1++;