[v4,2/7] printk: initialize syslog_prev and console_prev

Two global variables, "syslog_prev" and "console_prev", maintain a
copy of the flags value used in the log record most recently
formatted for syslog or the console, respectively.

Initially there is no previous formatted log record, and these
variables simply have an initial value 0.  And occasionally log
records can get consumed at a rate such that syslog or the console
can't keep up, in which case those variables (along with their
corresponding position variables) must be reset.  Here too, they're
reset to 0.

This patch changes it so the initial value used is LOG_NEWLINE.
That is, if we know nothing about the prevously-formatted log
record, we can assume it was complete, and ended with a newline.

This is being done to support the next patch.  Initializing
these variables this way makes LOG_NEWLINE and LOG_CONT be
mutually exclusive, and the next patch uses that fact to simplify
some logic.

Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Petr Mládek <pmladek@suse.cz>
---
 kernel/printk/printk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On Fri 2014-07-18 16:28:00, Alex Elder wrote:
> Two global variables, "syslog_prev" and "console_prev", maintain a
> copy of the flags value used in the log record most recently
> formatted for syslog or the console, respectively.
> 
> Initially there is no previous formatted log record, and these
> variables simply have an initial value 0.  And occasionally log
> records can get consumed at a rate such that syslog or the console
> can't keep up, in which case those variables (along with their
> corresponding position variables) must be reset.  Here too, they're
> reset to 0.
> 
> This patch changes it so the initial value used is LOG_NEWLINE.
> That is, if we know nothing about the prevously-formatted log
> record, we can assume it was complete, and ended with a newline.
> 
> This is being done to support the next patch.  Initializing
> these variables this way makes LOG_NEWLINE and LOG_CONT be
> mutually exclusive, and the next patch uses that fact to simplify
> some logic.

I have decided to double check it and found more locations where the flags were
reset to 0:

      + "prev" variable in syslog_print_all()
      + "prev" variable in kmsg_dump_get_buffer()


Also I think that we should not reset it to LOG_NEWLINE when the
message is gone. I would suggest to do

	&= (LOG_CONT | LOG_NEWLINE);

I mean to preserve the status of the last copied/printed piece. Then
we could put there "\n" when the last proceed piece was continuous and
the next one starts with prefix.

The only exception would be "console_prev" in  console_unlock()
because there we already add "\n" in the "** %u printk messages
dropped **" warning.

How does that sound?


Best Regards,
Petr


> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Petr Mládek <pmladek@suse.cz>
> ---
>  kernel/printk/printk.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ffc9928..4034a88 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -236,7 +236,7 @@ DECLARE_WAIT_QUEUE_HEAD(log_wait);
>  /* the next printk record to read by syslog(READ) or /proc/kmsg */
>  static u64 syslog_seq;
>  static u32 syslog_idx;
> -static enum log_flags syslog_prev;
> +static enum log_flags syslog_prev = LOG_NEWLINE;
>  static size_t syslog_partial;
>  
>  /* index and sequence number of the first record stored in the buffer */
> @@ -250,7 +250,7 @@ static u32 log_next_idx;
>  /* the next printk record to write to the console */
>  static u64 console_seq;
>  static u32 console_idx;
> -static enum log_flags console_prev;
> +static enum log_flags console_prev = LOG_NEWLINE;
>  
>  /* the next printk record to read after the last 'clear' command */
>  static u64 clear_seq;
> @@ -1071,7 +1071,7 @@ static int syslog_print(char __user *buf, int size)
>  			/* messages are gone, move to first one */
>  			syslog_seq = log_first_seq;
>  			syslog_idx = log_first_idx;
> -			syslog_prev = 0;
> +			syslog_prev = LOG_NEWLINE;
>  			syslog_partial = 0;
>  		}
>  		if (syslog_seq == log_next_seq) {
> @@ -1301,7 +1301,7 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  			/* messages are gone, move to first one */
>  			syslog_seq = log_first_seq;
>  			syslog_idx = log_first_idx;
> -			syslog_prev = 0;
> +			syslog_prev = LOG_NEWLINE;
>  			syslog_partial = 0;
>  		}
>  		if (from_file) {
> @@ -2149,7 +2149,7 @@ again:
>  			/* messages are gone, move to first one */
>  			console_seq = log_first_seq;
>  			console_idx = log_first_idx;
> -			console_prev = 0;
> +			console_prev = LOG_NEWLINE;
>  		} else {
>  			len = 0;
>  		}
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 07/21/2014 05:01 AM, Petr Mládek wrote:
> On Fri 2014-07-18 16:28:00, Alex Elder wrote:
>> Two global variables, "syslog_prev" and "console_prev", maintain a
>> copy of the flags value used in the log record most recently
>> formatted for syslog or the console, respectively.
>>
>> Initially there is no previous formatted log record, and these
>> variables simply have an initial value 0.  And occasionally log
>> records can get consumed at a rate such that syslog or the console
>> can't keep up, in which case those variables (along with their
>> corresponding position variables) must be reset.  Here too, they're
>> reset to 0.
>>
>> This patch changes it so the initial value used is LOG_NEWLINE.
>> That is, if we know nothing about the prevously-formatted log
>> record, we can assume it was complete, and ended with a newline.
>>
>> This is being done to support the next patch.  Initializing
>> these variables this way makes LOG_NEWLINE and LOG_CONT be
>> mutually exclusive, and the next patch uses that fact to simplify
>> some logic.
> 
> I have decided to double check it and found more locations where the flags were
> reset to 0:
> 
>       + "prev" variable in syslog_print_all()
>       + "prev" variable in kmsg_dump_get_buffer()

Yes, you're right.  I'm glad you checked.  Some of those
don't matter much (because they're only measuring the size
of the needed buffer) but I'll do it on all of them for
consistency.

> Also I think that we should not reset it to LOG_NEWLINE when the
> message is gone. I would suggest to do
> 
> 	&= (LOG_CONT | LOG_NEWLINE);

What you're saying is that in the case we're resetting
because we couldn't keep up, we *do* in fact know something
about the previously-formatted message, so we should preserve
that.  That's an astute observation.  I like it, and will
implement it.

Thanks.  v5 will be coming out this morning.

					-Alex

> I mean to preserve the status of the last copied/printed piece. Then
> we could put there "\n" when the last proceed piece was continuous and
> the next one starts with prefix.
> 
> The only exception would be "console_prev" in  console_unlock()
> because there we already add "\n" in the "** %u printk messages
> dropped **" warning.
> 
> How does that sound?
> 
> 
> Best Regards,
> Petr
> 
> 
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> Reviewed-by: Petr Mládek <pmladek@suse.cz>
>> ---
>>  kernel/printk/printk.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index ffc9928..4034a88 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -236,7 +236,7 @@ DECLARE_WAIT_QUEUE_HEAD(log_wait);
>>  /* the next printk record to read by syslog(READ) or /proc/kmsg */
>>  static u64 syslog_seq;
>>  static u32 syslog_idx;
>> -static enum log_flags syslog_prev;
>> +static enum log_flags syslog_prev = LOG_NEWLINE;
>>  static size_t syslog_partial;
>>  
>>  /* index and sequence number of the first record stored in the buffer */
>> @@ -250,7 +250,7 @@ static u32 log_next_idx;
>>  /* the next printk record to write to the console */
>>  static u64 console_seq;
>>  static u32 console_idx;
>> -static enum log_flags console_prev;
>> +static enum log_flags console_prev = LOG_NEWLINE;
>>  
>>  /* the next printk record to read after the last 'clear' command */
>>  static u64 clear_seq;
>> @@ -1071,7 +1071,7 @@ static int syslog_print(char __user *buf, int size)
>>  			/* messages are gone, move to first one */
>>  			syslog_seq = log_first_seq;
>>  			syslog_idx = log_first_idx;
>> -			syslog_prev = 0;
>> +			syslog_prev = LOG_NEWLINE;
>>  			syslog_partial = 0;
>>  		}
>>  		if (syslog_seq == log_next_seq) {
>> @@ -1301,7 +1301,7 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>>  			/* messages are gone, move to first one */
>>  			syslog_seq = log_first_seq;
>>  			syslog_idx = log_first_idx;
>> -			syslog_prev = 0;
>> +			syslog_prev = LOG_NEWLINE;
>>  			syslog_partial = 0;
>>  		}
>>  		if (from_file) {
>> @@ -2149,7 +2149,7 @@ again:
>>  			/* messages are gone, move to first one */
>>  			console_seq = log_first_seq;
>>  			console_idx = log_first_idx;
>> -			console_prev = 0;
>> +			console_prev = LOG_NEWLINE;
>>  		} else {
>>  			len = 0;
>>  		}
>> -- 
>> 1.9.1
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/