[PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Chen Gang]

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().


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/panic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..c00b4ce 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -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;
-- 
1.7.7.6

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Chen Gang]

Hello Al Viro:

How about 2nd patch, is it correct?

I have finished my 2 patches which for an evaluation by you.  ;-)

Thanks.

On 10/05/2013 11:50 PM, Chen Gang wrote:
> 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().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/panic.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..c00b4ce 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -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;
> 


-- 
Chen Gang

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Alex Thorlton]

On Sat, Oct 05, 2013 at 11:50:39PM +0800, Chen Gang wrote:
> 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().
>

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 :/
 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/panic.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..c00b4ce 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -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;
> -- 
> 1.7.7.6

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Chen Gang]

On 10/08/2013 12:35 AM, Alex Thorlton wrote:
> On Sat, Oct 05, 2013 at 11:50:39PM +0800, Chen Gang wrote:
>> 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().
>>
> 
> 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.

>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/panic.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..c00b4ce 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -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;
>> -- 
>> 1.7.7.6
> 
> 


-- 
Chen Gang

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Andrew Morton]

On Sat, 05 Oct 2013 23:50:39 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> 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().
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/panic.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b6c482c..c00b4ce 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -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...

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Al Viro]

On Mon, Oct 07, 2013 at 05:27:45PM -0700, Andrew Morton wrote:

> hm, that code is a bit crufty.
> 
>  - Why is `buf' static?  It could be on-stack.

No, it couldn't - check the callers...

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Chen Gang]

On 10/08/2013 08:27 AM, Andrew Morton wrote:
> On Sat, 05 Oct 2013 23:50:39 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> 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().
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/panic.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b6c482c..c00b4ce 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -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.
> 

Just like Al Viro's reply ('buf' will be returned to caller).


> - 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.


> - 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).

> 
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Andrew Morton]

On Tue, 08 Oct 2013 09:10:37 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> > - 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.
> 
> 
> > - 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).

It's hardly a huge issue, but I'd do something along the lines of

--- a/kernel/panic.c~a
+++ a/kernel/panic.c
@@ -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.

Re: [PATCH] kernel/panic.c: reduce 1 byte usage for print tainted buffer.

[Chen Gang]

On 10/08/2013 09:25 AM, Andrew Morton wrote:
> On Tue, 08 Oct 2013 09:10:37 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>>> - 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.
>>
>>
>>> - 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).
> 
> It's hardly a huge issue, but I'd do something along the lines of
> 
> --- a/kernel/panic.c~a
> +++ a/kernel/panic.c
> @@ -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;
>  }
> 

Theoretically, your implementation is better than the original one.


> 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.
-- 
Chen Gang