[PATCH] printk: Modify operators of printed_len

[PATCH] printk: Modify operators of printed_len

[Pierre Kuo]

In 8b1742c9c207, we remove printk-recursion detection code in
vprintk_emit(), where it is the first place that printed_len calculated.
After removing above detection, it seems we can directly assign the
result of log_output to printed_len.

Signed-off-by: Pierre Kuo <vichy.kuo@gmail.com>
---
 kernel/printk/printk.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..16f3a61 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (dict)
 		lflags |= LOG_PREFIX|LOG_NEWLINE;
 
-	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
+	printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
 
 	logbuf_unlock_irqrestore(flags);
 
-- 
1.7.9.5

Re: [PATCH] printk: Modify operators of printed_len

[Joe Perches]

On Sat, 2017-07-08 at 00:30 +0800, Pierre Kuo wrote:
> In 8b1742c9c207, we remove printk-recursion detection code in
> vprintk_emit(), where it is the first place that printed_len calculated.
> After removing above detection, it seems we can directly assign the
> result of log_output to printed_len.
[]
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
[]
> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	if (dict)
>  		lflags |= LOG_PREFIX|LOG_NEWLINE;
>  
> -	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
> +	printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);

If this is appropriate, this should also remove the
initialization of printed_len and perhaps rename it too.

Re: [PATCH] printk: Modify operators of printed_len

[pierre kuo]

hi Joe:
2017-07-08 1:12 GMT+08:00 Joe Perches <joe@perches.com>:
> On Sat, 2017-07-08 at 00:30 +0800, Pierre Kuo wrote:
>> In 8b1742c9c207, we remove printk-recursion detection code in
>> vprintk_emit(), where it is the first place that printed_len calculated.
>> After removing above detection, it seems we can directly assign the
>> result of log_output to printed_len.
> []
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> []
>> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>>       if (dict)
>>               lflags |= LOG_PREFIX|LOG_NEWLINE;
>>
>> -     printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
>> +     printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
>
> If this is appropriate, this should also remove the
> initialization of printed_len and perhaps rename it too.
I cannot quite understand the reason why need to rename.
printed_len seems meet the meaning we expect for here.

thanks for your friendly comment.

Re: [PATCH] printk: Modify operators of printed_len

[Joe Perches]

On Sat, 2017-07-08 at 04:32 +0800, pierre kuo wrote:
> hi Joe:

Hello Pierre.

> 2017-07-08 1:12 GMT+08:00 Joe Perches <joe@perches.com>:
> > On Sat, 2017-07-08 at 00:30 +0800, Pierre Kuo wrote:
> > > In 8b1742c9c207, we remove printk-recursion detection code in
> > > vprintk_emit(), where it is the first place that printed_len calculated.
> > > After removing above detection, it seems we can directly assign the
> > > result of log_output to printed_len.
> > 
> > []
> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > 
> > []
> > > @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> > >       if (dict)
> > >               lflags |= LOG_PREFIX|LOG_NEWLINE;
> > > 
> > > -     printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
> > > +     printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
> > 
> > If this is appropriate, this should also remove the
> > initialization of printed_len and perhaps rename it too.
> 
> I cannot quite understand the reason why need to rename.
> printed_len seems meet the meaning we expect for here.

Verbosity.  To me, len would be adequate.

Anyway, the real point was the declaration of printed_len could
remove the " = 0" as it's now only set once.

cheers, Joe

Re: [PATCH] printk: Modify operators of printed_len

[pierre kuo]

hi Joe
>> > []
>> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> >
>> > []
>> > > @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>> > >       if (dict)
>> > >               lflags |= LOG_PREFIX|LOG_NEWLINE;
>> > >
>> > > -     printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
>> > > +     printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
>> >
>> > If this is appropriate, this should also remove the
>> > initialization of printed_len and perhaps rename it too.
>>
>> I cannot quite understand the reason why need to rename.
>> printed_len seems meet the meaning we expect for here.
>
> Verbosity.  To me, len would be adequate.
>
> Anyway, the real point was the declaration of printed_len could
> remove the " = 0" as it's now only set once.
Got it and I will resend the patch again.

Appreciate your kind advice.

[PATCH] printk: Modify operators of printed_len

[Pierre Kuo]

In 8b1742c9c207, we remove printk-recursion detection code in
vprintk_emit(), where it is the first place that printed_len calculated.
After removing above detection, it seems we can directly assign the
result of log_output to printed_len.

Signed-off-by: Pierre Kuo <vichy.kuo@gmail.com>
---
 kernel/printk/printk.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..a2a8cac 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1701,7 +1701,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	size_t text_len = 0;
 	enum log_flags lflags = 0;
 	unsigned long flags;
-	int printed_len = 0;
+	int printed_len;
 	bool in_sched = false;
 
 	if (level == LOGLEVEL_SCHED) {
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (dict)
 		lflags |= LOG_PREFIX|LOG_NEWLINE;
 
-	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
+	printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
 
 	logbuf_unlock_irqrestore(flags);
 
-- 
1.7.9.5

Re: [PATCH] printk: Modify operators of printed_len

[Sergey Senozhatsky]

On (07/08/17 10:51), Pierre Kuo wrote:
> In 8b1742c9c207, we remove printk-recursion detection code in
> vprintk_emit(), where it is the first place that printed_len calculated.
> After removing above detection, it seems we can directly assign the
> result of log_output to printed_len.
> 
> Signed-off-by: Pierre Kuo <vichy.kuo@gmail.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> ---
>  kernel/printk/printk.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fc47863..a2a8cac 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1701,7 +1701,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	size_t text_len = 0;
>  	enum log_flags lflags = 0;
>  	unsigned long flags;
> -	int printed_len = 0;
> +	int printed_len;
>  	bool in_sched = false;
>  
>  	if (level == LOGLEVEL_SCHED) {
> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	if (dict)
>  		lflags |= LOG_PREFIX|LOG_NEWLINE;
>  
> -	printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len);
> +	printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len);
>  
>  	logbuf_unlock_irqrestore(flags);
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH] printk: Modify operators of printed_len

[Petr Mladek]

On Sat 2017-07-08 10:51:13, Pierre Kuo wrote:
> In 8b1742c9c207, we remove printk-recursion detection code in
> vprintk_emit(), where it is the first place that printed_len calculated.
> After removing above detection, it seems we can directly assign the
> result of log_output to printed_len.
> 
> Signed-off-by: Pierre Kuo <vichy.kuo@gmail.com>

Great catch!

I just noticed that the same applies also to text_len
variable. Well, it was caused by another commit ddb9baa822265b55
("printk: report lost messages in printk safe/nmi contexts").
Could you please send a patch for this as well?

I would personally fix both variables in a single patch. But
I do not have a strong opinion about it.


This seems to be your first patch sent to the kernel mailing list.
Let me share some hints that might help you to handle more complex
patchsets ;-)

There is a standard format how to reference older commits. It is
'commit <12+ chars of sha1> ("<title line>")', see my comment above
for an example.

A good practice is to run ./scripts/checkpatch.pl <patch> before
you send the patch. Well, you need to use a common sense and ignore
false positives or hints that make a particular patch less readable
in the end.

Also it is handy to bump the version of the patch when it is
updated, e.g. use [PATCH v2] in the subject. People also
summarize changes against the previous version(s) below
the --- line. Well, this is more useful when there is a longer
delay between the versions and the changes are more complicated.

Best Regards,
Petr

Re: [PATCH] printk: Modify operators of printed_len

[pierre kuo]

hi Petr
> I just noticed that the same applies also to text_len
> variable. Well, it was caused by another commit ddb9baa822265b55
> ("printk: report lost messages in printk safe/nmi contexts").
> Could you please send a patch for this as well?
sure and it is my pleasure.

>
> This seems to be your first patch sent to the kernel mailing list.
Yes :-)

> There is a standard format how to reference older commits. It is
> 'commit <12+ chars of sha1> ("<title line>")', see my comment above
> for an example.
>
> A good practice is to run ./scripts/checkpatch.pl <patch> before
> you send the patch. Well, you need to use a common sense and ignore
> false positives or hints that make a particular patch less readable
> in the end.
>
> Also it is handy to bump the version of the patch when it is
> updated, e.g. use [PATCH v2] in the subject. People also
> summarize changes against the previous version(s) below
> the --- line. Well, this is more useful when there is a longer
> delay between the versions and the changes are more complicated.
Really appreciate hints you provided and I will send the v2 patch soon.

Best Regards.