[v5,3/7] printk: initialize syslog_prev and console_prev

Message ID 1405947761-26004-4-git-send-email-elder@linaro.org
State New
Headers show

Commit Message

Alex Elder July 21, 2014, 1:02 p.m.
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.
One exception is that occasionally we reset our syslog or console
(etc.) position variables.  In that case the previously-formatted
record flags value is still valid, so we preserve that information.

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 | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

Comments

Alex Elder July 22, 2014, 11:48 a.m. | #1
On 07/21/2014 08:02 AM, 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.
> One exception is that occasionally we reset our syslog or console
> (etc.) position variables.  In that case the previously-formatted
> record flags value is still valid, so we preserve that information.
> 
> 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>

I have one change I'd like to suggest on this one.

Petr, could you offer your opinion?


> ---
>  kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 9f11eab..2f43116 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c


. . .

> @@ -2156,10 +2168,14 @@ again:
>  				      "%s** %u printk messages dropped **\n",
>  				      (console_prev & LOG_CONT) ? "\n" : "",
>  				      (unsigned)(log_first_seq - console_seq));
> -			/* messages are gone, move to first one */
> +			/*
> +			 * Messages are gone, move to first one.
> +			 * Don't discard what we know about the
> +			 * previously-formatted record.
> +			 */
>  			console_seq = log_first_seq;
>  			console_idx = log_first_idx;
> -			console_prev = 0;
> +			console_prev &= LOG_NEWLINE|LOG_CONT;

In this one spot, I think console_prev should simply be
initialized with LOG_NEWLINE.

The reason is that the "printk messages dropped" message will
be inserted into the formatted output, and is hence the last
formatted line.  And that message is (now) terminated with
a newline.

					-Alex

>  		} else {
>  			len = 0;
>  		}

. . .

--
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/
Petr Mladek July 22, 2014, 12:01 p.m. | #2
On Tue 2014-07-22 06:48:22, Alex Elder wrote:
> On 07/21/2014 08:02 AM, 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.
> > One exception is that occasionally we reset our syslog or console
> > (etc.) position variables.  In that case the previously-formatted
> > record flags value is still valid, so we preserve that information.
> > 
> > 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>
> 
> I have one change I'd like to suggest on this one.
> 
> Petr, could you offer your opinion?
> 
> 
> > ---
> >  kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 9f11eab..2f43116 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> 
> 
> . . .
> 
> > @@ -2156,10 +2168,14 @@ again:
> >  				      "%s** %u printk messages dropped **\n",
> >  				      (console_prev & LOG_CONT) ? "\n" : "",
> >  				      (unsigned)(log_first_seq - console_seq));
> > -			/* messages are gone, move to first one */
> > +			/*
> > +			 * Messages are gone, move to first one.
> > +			 * Don't discard what we know about the
> > +			 * previously-formatted record.
> > +			 */
> >  			console_seq = log_first_seq;
> >  			console_idx = log_first_idx;
> > -			console_prev = 0;
> > +			console_prev &= LOG_NEWLINE|LOG_CONT;
> 
> In this one spot, I think console_prev should simply be
> initialized with LOG_NEWLINE.
> 
> The reason is that the "printk messages dropped" message will
> be inserted into the formatted output, and is hence the last
> formatted line.  And that message is (now) terminated with
> a newline.

Sure. Great catch! I actually mentioned it when commenting v4, see
https://lkml.org/lkml/2014/7/21/153 . But I forgot it today :-/


Best Regards,
Petr
--
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/

Patch hide | download patch | download mbox

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9f11eab..2f43116 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;
@@ -1078,10 +1078,14 @@  static int syslog_print(char __user *buf, int size)
 
 		raw_spin_lock_irq(&logbuf_lock);
 		if (syslog_seq < log_first_seq) {
-			/* messages are gone, move to first one */
+			/*
+			 * Messages are gone, move to first one.
+			 * Don't discard what we know about the
+			 * previously-formatted record.
+			 */
 			syslog_seq = log_first_seq;
 			syslog_idx = log_first_idx;
-			syslog_prev = 0;
+			syslog_prev &= LOG_NEWLINE|LOG_CONT;
 			syslog_partial = 0;
 		}
 		if (syslog_seq == log_next_seq) {
@@ -1154,7 +1158,7 @@  static int syslog_print_all(char __user *buf, int size, bool clear)
 		 */
 		seq = clear_seq;
 		idx = clear_idx;
-		prev = 0;
+		prev = LOG_NEWLINE;
 		while (seq < log_next_seq) {
 			struct printk_log *msg = log_from_idx(idx);
 
@@ -1167,7 +1171,7 @@  static int syslog_print_all(char __user *buf, int size, bool clear)
 		/* move first record forward until length fits into the buffer */
 		seq = clear_seq;
 		idx = clear_idx;
-		prev = 0;
+		prev = LOG_NEWLINE;
 		while (len > size && seq < log_next_seq) {
 			struct printk_log *msg = log_from_idx(idx);
 
@@ -1203,10 +1207,14 @@  static int syslog_print_all(char __user *buf, int size, bool clear)
 			raw_spin_lock_irq(&logbuf_lock);
 
 			if (seq < log_first_seq) {
-				/* messages are gone, move to next one */
+				/*
+				 * Messages are gone, move to first one.
+				 * Don't discard what we know about the
+				 * previously-formatted record.
+				 */
 				seq = log_first_seq;
 				idx = log_first_idx;
-				prev = 0;
+				prev &= LOG_NEWLINE|LOG_CONT;
 			}
 		}
 	}
@@ -1308,10 +1316,14 @@  int do_syslog(int type, char __user *buf, int len, bool from_file)
 	case SYSLOG_ACTION_SIZE_UNREAD:
 		raw_spin_lock_irq(&logbuf_lock);
 		if (syslog_seq < log_first_seq) {
-			/* messages are gone, move to first one */
+			/*
+			 * Messages are gone, move to first one.
+			 * Don't discard what we know about the
+			 * previously-formatted record.
+			 */
 			syslog_seq = log_first_seq;
 			syslog_idx = log_first_idx;
-			syslog_prev = 0;
+			syslog_prev &= LOG_NEWLINE|LOG_CONT;
 			syslog_partial = 0;
 		}
 		if (from_file) {
@@ -2156,10 +2168,14 @@  again:
 				      "%s** %u printk messages dropped **\n",
 				      (console_prev & LOG_CONT) ? "\n" : "",
 				      (unsigned)(log_first_seq - console_seq));
-			/* messages are gone, move to first one */
+			/*
+			 * Messages are gone, move to first one.
+			 * Don't discard what we know about the
+			 * previously-formatted record.
+			 */
 			console_seq = log_first_seq;
 			console_idx = log_first_idx;
-			console_prev = 0;
+			console_prev &= LOG_NEWLINE|LOG_CONT;
 		} else {
 			len = 0;
 		}
@@ -2873,7 +2889,7 @@  bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
 	/* calculate length of entire buffer */
 	seq = dumper->cur_seq;
 	idx = dumper->cur_idx;
-	prev = 0;
+	prev = LOG_NEWLINE;
 	while (seq < dumper->next_seq) {
 		struct printk_log *msg = log_from_idx(idx);
 
@@ -2886,7 +2902,7 @@  bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
 	/* move first record forward until length fits into the buffer */
 	seq = dumper->cur_seq;
 	idx = dumper->cur_idx;
-	prev = 0;
+	prev = LOG_NEWLINE;
 	while (l > size && seq < dumper->next_seq) {
 		struct printk_log *msg = log_from_idx(idx);