* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
@ 2015-03-16 18:16 ` Borislav Petkov
2015-03-17 7:46 ` Ingo Molnar
2015-03-23 12:23 ` [tip:x86/boot] x86/boot: Standardize strcmp() tip-bot for Arjun Sreedharan
2 siblings, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2015-03-16 18:16 UTC (permalink / raw)
To: Arjun Sreedharan
Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel
On Mon, Mar 16, 2015 at 09:07:47PM +0530, Arjun Sreedharan wrote:
> 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 <arjun024@gmail.com>
> ---
> arch/x86/boot/string.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> index 493f3fd..318b846 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
2015-03-16 18:16 ` Borislav Petkov
@ 2015-03-17 7:46 ` Ingo Molnar
2015-03-17 14:13 ` Arjun Sreedharan
2015-03-17 14:28 ` Borislav Petkov
2015-03-23 12:23 ` [tip:x86/boot] x86/boot: Standardize strcmp() tip-bot for Arjun Sreedharan
2 siblings, 2 replies; 11+ messages in thread
From: Ingo Molnar @ 2015-03-17 7:46 UTC (permalink / raw)
To: Arjun Sreedharan
Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel
* Arjun Sreedharan <arjun024@gmail.com> wrote:
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-17 7:46 ` Ingo Molnar
@ 2015-03-17 14:13 ` Arjun Sreedharan
2015-03-18 1:36 ` Bernd Petrovitsch
2015-03-19 17:34 ` H. Peter Anvin
2015-03-17 14:28 ` Borislav Petkov
1 sibling, 2 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-17 14:13 UTC (permalink / raw)
To: Ingo Molnar
Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-kernel
On 17 March 2015 at 13:16, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Arjun Sreedharan <arjun024@gmail.com> wrote:
>
>> 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
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-17 14:13 ` Arjun Sreedharan
@ 2015-03-18 1:36 ` Bernd Petrovitsch
2015-03-18 18:10 ` Arjun Sreedharan
2015-03-18 18:37 ` Arjun Sreedharan
2015-03-19 17:34 ` H. Peter Anvin
1 sibling, 2 replies; 11+ messages in thread
From: Bernd Petrovitsch @ 2015-03-18 1:36 UTC (permalink / raw)
To: Arjun Sreedharan
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
linux-kernel
On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
[...]
> On a related note, IMO strcmp() should return {-1,0,1} since many
> programmers just expect this behavior. just my opinion.
-ENOPATCH.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-18 1:36 ` Bernd Petrovitsch
@ 2015-03-18 18:10 ` Arjun Sreedharan
2015-03-18 18:37 ` Arjun Sreedharan
1 sibling, 0 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-18 18:10 UTC (permalink / raw)
To: Bernd Petrovitsch
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
linux-kernel
On 18 March 2015 at 07:06, Bernd Petrovitsch <bernd@petrovitsch.priv.at> wrote:
> On Die, 2015-03-17 at 19:43 +0530, Arjun Sreedharan wrote:
> [...]
>> 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 <arjun024@gmail.com>
---
arch/arm/boot/compressed/string.c | 2 +-
arch/x86/boot/string.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/compressed/string.c
b/arch/arm/boot/compressed/string.c
index 36e53ef..e48df86 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -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;
}
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 318b846..6eb333e 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -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++;
}
--
>
> 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
>
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] x86,boot: standardize strcmp()
2015-03-18 1:36 ` Bernd Petrovitsch
2015-03-18 18:10 ` Arjun Sreedharan
@ 2015-03-18 18:37 ` Arjun Sreedharan
1 sibling, 0 replies; 11+ messages in thread
From: Arjun Sreedharan @ 2015-03-18 18:37 UTC (permalink / raw)
To: Bernd Petrovitsch
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86,
linux-kernel
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 <arjun024@gmail.com>
---
arch/arm/boot/compressed/string.c | 2 +-
arch/x86/boot/string.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
index 36e53ef..e48df86 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -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;
}
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 318b846..6eb333e 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -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++;
}
--
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-17 14:13 ` Arjun Sreedharan
2015-03-18 1:36 ` Bernd Petrovitsch
@ 2015-03-19 17:34 ` H. Peter Anvin
2015-03-20 11:42 ` Bernd Petrovitsch
1 sibling, 1 reply; 11+ messages in thread
From: H. Peter Anvin @ 2015-03-19 17:34 UTC (permalink / raw)
To: Arjun Sreedharan, Ingo Molnar
Cc: Thomas Gleixner, Ingo Molnar, x86, linux-kernel
On 03/17/2015 07:13 AM, Arjun Sreedharan wrote:
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-19 17:34 ` H. Peter Anvin
@ 2015-03-20 11:42 ` Bernd Petrovitsch
0 siblings, 0 replies; 11+ messages in thread
From: Bernd Petrovitsch @ 2015-03-20 11:42 UTC (permalink / raw)
To: Arjun Sreedharan
Cc: H. Peter Anvin, Ingo Molnar, Thomas Gleixner, Ingo Molnar, x86,
linux-kernel
On Don, 2015-03-19 at 10:34 -0700, H. Peter Anvin wrote:
> On 03/17/2015 07:13 AM, Arjun Sreedharan wrote:
> > 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.
> 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
--
"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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86,boot: standardize strcmp()
2015-03-17 7:46 ` Ingo Molnar
2015-03-17 14:13 ` Arjun Sreedharan
@ 2015-03-17 14:28 ` Borislav Petkov
1 sibling, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2015-03-17 14:28 UTC (permalink / raw)
To: Ingo Molnar
Cc: Arjun Sreedharan, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
x86, linux-kernel
On Tue, Mar 17, 2015 at 08:46:08AM +0100, Ingo Molnar wrote:
> I'd also add the following to the changelog:
Added:
---
From: Arjun Sreedharan <arjun024@gmail.com>
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 <arjun024@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Link: http://lkml.kernel.org/r/1426520267-1803-1-git-send-email-arjun024@gmail.com
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/boot/string.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 493f3fd9f139..318b8465d302 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -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++;
--
2.3.3
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [tip:x86/boot] x86/boot: Standardize strcmp()
2015-03-16 15:37 [PATCH] x86,boot: standardize strcmp() Arjun Sreedharan
2015-03-16 18:16 ` Borislav Petkov
2015-03-17 7:46 ` Ingo Molnar
@ 2015-03-23 12:23 ` tip-bot for Arjun Sreedharan
2 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Arjun Sreedharan @ 2015-03-23 12:23 UTC (permalink / raw)
To: linux-tip-commits; +Cc: hpa, arjun024, mingo, linux-kernel, bp, tglx
Commit-ID: 1c1d046be692493d00a4831d4fbc266745008e09
Gitweb: http://git.kernel.org/tip/1c1d046be692493d00a4831d4fbc266745008e09
Author: Arjun Sreedharan <arjun024@gmail.com>
AuthorDate: Mon, 16 Mar 2015 21:07:47 +0530
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 23 Mar 2015 10:24:12 +0100
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 <arjun024@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1426520267-1803-1-git-send-email-arjun024@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/boot/string.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 493f3fd..318b846 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -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++;
^ permalink raw reply related [flat|nested] 11+ messages in thread