kernel/panic.c: reduce 1 byte usage for print tainted buffer.
PATCH 1/1 kernel/panic.c: reduce 1 byte usage for print tainted buffer.
sizeof("Tainted: ") already counts '\0', and after first sprintf(), 's' will start from the current string end (its' value is '\0').
So need not add additional 1 byte for maximized usage of 'buf' in print_tainted().
--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/panic.c +1 -1
@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];if (tainted_mask) {char *s;
Signature · 1 line
1.7.7.6
Hello Al Viro:
How about 2nd patch, is it correct?
I have finished my 2 patches which for an evaluation by you. ;-)
Thanks.
Quoted 26 lines · Chen Gang
sizeof("Tainted: ")already counts '\0', and after firstsprintf(), 's' will start from the current string end (its' value is '\0').So need not add additional 1 byte for maximized usage of 'buf' in
print_tainted().Signed-off-by Chen Gang <[email protected]>--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/panic.c +1
-1@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];if (tainted_mask) {char *s;
Signature · 1 line
Chen Gang
Quoted 6 lines · Chen Gang
sizeof("Tainted: ")already counts '\0', and after firstsprintf(), 's' will start from the current string end (its' value is '\0').So need not add additional 1 byte for maximized usage of 'buf' in
print_tainted().
Looks fine to me; nothing too crazy going on here. By the way, Robin isn't with us here at SGI any more. He's at Cray, now, but I don't know his e-mail there :/
Quoted 21 lines
Signed-off-by Chen Gang <[email protected]>--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/panic.c +1
-1@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];if (tainted_mask) {char *s;Signature · 1 line
1.7.7.6
Quoted 4 lines · Alex Thorlton
Quoted 3 lines · Chen Gang
sizeof("Tainted: ")already counts '\0', and after firstsprintf(), 's' will start from the current string end (its' value is '\0').
Quoted 8 lines
Quoted 3 lines
So need not add additional 1 byte for maximized usage of 'buf' in
print_tainted().Looks fine to me; nothing too crazy going on here. By the way, Robin isn't with us here at SGI any more. He's at Cray, now, but I don't know his e-mail there :/
OK, thanks.
Quoted 23 lines
Quoted 21 lines
Signed-off-by Chen Gang <[email protected]>--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/panic.c +1
-1@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];if (tainted_mask) {char *s;Signature · 1 line
1.7.7.6
Signature · 1 line
Chen Gang
Quoted 17 lines · On Sat, 05 Oct 2013 23:50:39 +0800 Chen Gang <[email protected]> wrote:
sizeof("Tainted: ")already counts '\0', and after firstsprintf(), 's' will start from the current string end (its' value is '\0').So need not add additional 1 byte for maximized usage of 'buf' in
print_tainted().Signed-off-by Chen Gang <[email protected]>--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/panic.c +0
-0
kernel/panic.c static const struct tnt tnts[] = { · line 239
Quoted 8 lines
kernel/panic.c +1
-1@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];if (tainted_mask) {char *s;
hm, that code is a bit crufty.
- Why is `buf' static? It could be on-stack.
- Requires that the two literal "Tainted: " strings be kept in sync.
- Assumes that strlen("Not tainted") <= strlen("Tainted") + ARRAY_SIZE(tnts). Which is true, but might not be if someone makes changes...
Quoted 3 lines · Andrew Morton
hm, that code is a bit crufty.
- Why is `buf' static? It could be on-stack.
No, it couldn't - check the callers...
Quoted 25 lines · Andrew Morton
Quoted 23 lines · On Sat, 05 Oct 2013 23:50:39 +0800 Chen Gang <[email protected]> wrote:
sizeof("Tainted: ")already counts '\0', and after firstsprintf(), 's' will start from the current string end (its' value is '\0').So need not add additional 1 byte for maximized usage of 'buf' in
print_tainted().Signed-off-by Chen Gang <[email protected]>--- kernel/panic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/panic.c +1
-1@@ -233,7 +233,7 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " ) +1 ]; +static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )];
Quoted 7 lines
kernel/panic.c static const struct tnt tnts[] = { · line 239
Quoted 2 lines
kernel/panic.c +0
-0if (tainted_mask) {char *s;hm, that code is a bit crufty.
- Why is `buf' static? It could be on-stack.
Just like Al Viro's reply ('buf' will be returned to caller).
Quoted 2 lines
- Requires that the two literal "Tainted: " strings be kept in sync.
Hmm... if more than 2, we should use a macro instead of, else (within 2), especially they are near by, we can still let it hard coded, I feel that will more straightful for readers and writers.
Quoted 4 lines
- Assumes that
strlen("Not tainted")<=strlen("Tainted")+ARRAY_SIZE(tnts). Which is true, but might not be if someone makes changes...
Hmm... it use snprintf() instead of sprintf(), although I feel better using scnprintf() instead of.
This string can be trucated, and scnprintf() is more suitable for this kind of string. And snprintf() is for the string which can not be truncated (will return the ideal length to notify the caller).
Quoted 3 lines
Thanks.
Signature · 1 line
Chen Gang
Quoted 13 lines · On Tue, 08 Oct 2013 09:10:37 +0800 Chen Gang <[email protected]> wrote:
Quoted 2 lines
- Requires that the two literal "Tainted: " strings be kept in sync.
Hmm... if more than 2, we should use a macro instead of, else (within 2), especially they are near by, we can still let it hard coded, I feel that will more straightful for readers and writers.
Quoted 4 lines
- Assumes that
strlen("Not tainted")<=strlen("Tainted")+ARRAY_SIZE(tnts). Which is true, but might not be if someone makes changes...
Quoted 6 lines
Hmm... it use
snprintf()instead ofsprintf(), although I feel better usingscnprintf()instead of.This string can be trucated, and
scnprintf()is more suitable for this kind of string. Andsnprintf()is for the string which can not be truncated (will return the ideal length to notify the caller).
It's hardly a huge issue, but I'd do something along the lines of
kernel/panic.c renamed from kernel/panic.c~a +6 -3
@@ -233,13 +233,16 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )]; +static const char tainted[] ="Tainted: " ; +static const char not_tainted[] ="Not tainted: " ; +static char buf[ARRAY_SIZE (tnts) + +max (sizeof (tainted),sizeof (not_tainted))];if (tainted_mask) {char *s;int i; - s = buf +sprintf (buf,"Tainted: " ); + s = buf +sprintf (buf, tainted);for (i =0 ; i <ARRAY_SIZE (tnts); i++) {const struct tnt *t = &tnts[i]; *s++ = test_bit(t->bit, &tainted_mask) ?
@@ -247,7 +250,7 @@ const char *print_tainted(void) } *s =0 ; }else -snprintf (buf,sizeof (buf),"Not tainted" ); +snprintf (buf,sizeof (buf), not_tainted);return buf; }
Except that doesn't compile because of our fancy max().
Maybe we have a compile-time-evaluable max which does plain old ((a < b) ? b : a), not sure... I don't think it's worth bothering about - leave it be.
Quoted 47 lines · Andrew Morton
Quoted 13 lines · On Tue, 08 Oct 2013 09:10:37 +0800 Chen Gang <[email protected]> wrote:
Quoted 2 lines
- Requires that the two literal "Tainted: " strings be kept in sync.
Hmm... if more than 2, we should use a macro instead of, else (within 2), especially they are near by, we can still let it hard coded, I feel that will more straightful for readers and writers.
Quoted 4 lines
- Assumes that
strlen("Not tainted")<=strlen("Tainted")+ARRAY_SIZE(tnts). Which is true, but might not be if someone makes changes...Quoted 6 lines
Hmm... it use
snprintf()instead ofsprintf(), although I feel better usingscnprintf()instead of.This string can be trucated, and
scnprintf()is more suitable for this kind of string. Andsnprintf()is for the string which can not be truncated (will return the ideal length to notify the caller).It's hardly a huge issue, but I'd do something along the lines of
kernel/panic.c renamed from kernel/panic.c~a +5
-2@@ -233,13 +233,16 @@ static const struct tnt tnts[] = {*/ const char *print_tainted (void ) { -static char buf[ARRAY_SIZE (tnts) +sizeof ("Tainted: " )]; +static const char tainted[] ="Tainted: " ; +static const char not_tainted[] ="Not tainted: " ; +static char buf[ARRAY_SIZE (tnts) + +max (sizeof (tainted),sizeof (not_tainted))];if (tainted_mask) {char *s;int i; - s = buf +sprintf (buf,"Tainted: " ); + s = buf +sprintf (buf, tainted);for (i =0 ; i <ARRAY_SIZE (tnts); i++) {const struct tnt *t = &tnts[i]; *s++ = test_bit(t->bit, &tainted_mask) ?
kernel/panic.c const char *print_tainted(void) · line 256
Quoted 8 lines
kernel/panic.c renamed from kernel/panic.c~a +1
-1@@ -247,7 +250,7 @@ const char *print_tainted(void) } *s =0 ; }else -snprintf (buf,sizeof (buf),"Not tainted" ); +snprintf (buf,sizeof (buf), not_tainted);return buf; }
Theoretically, your implementation is better than the original one.
Quoted 8 lines
Except that doesn't compile because of our fancy
max().Maybe we have a compile-time-evaluable max which does plain old ((a < b) ? b : a), not sure... I don't think it's worth bothering about - leave it be.
Excuse me, I am not quite understand your meaning :-(
Hmm... at least, if max() will be OK, I support your fixing.
- in my memory, the old C compiler may not recognize "? :" in array. maybe it is not old version ansi C standard requirements? (at least, it is rarely used in history).
- I guess: "max()" may be just the reason why original author don't implement it like you have done (maybe at that time, the C compiler did not support it).
Thanks.
Signature · 1 line
Chen Gang