[2/4] printk: honor LOG_PREFIX in devkmsg_read()

Message ID 1405531620-9983-3-git-send-email-elder@linaro.org
State New
Headers show

Commit Message

Alex Elder July 16, 2014, 5:26 p.m.
In devkmsg_read(), a variable "cont" holds a character that's used
to indicate whether a given log line is a "continuation", that is,
whether a log record should be merged with the one before or after
it.  If a record should be merged with its successor (but not its
predecessor) that character is 'c'.  And the line following such a
'c' log record is normally marked with a '+' to show it is continues
its predecessor.  Any other cases are marked '-', indicating the
log record stands on its own.

There is an exception.  If a log record is marked LOG_PREFIX, it
indicates that this record represents a new log entry, implicitly
terminating the predecessor--even if the predecessor would otherwise
have been continued.  So a record marked LOG_PREFIX (that is not
also marked LOG_CONT) should have '-' for its "cont" variable.

The logic that determines this "continuation" character has a bug
that gets that exceptional case wrong.

The specific case that produces the wrong result is when all of
these conditions are non-zero:
    user->prev & LOG_CONT
    msg->flags & LOG_PREFIX
    msg->flags & LOG_CONT
The bug is that despite the message's LOG_PREFIX flag, the
"cont" character is getting set to '+' rather than 'c'.

The problem is that the message's LOG_PREFIX flag is getting
ignored if its LOG_CONT flag is also set.  Rearrange the logic
here to produce the correct result.

The following table concisely defines the problem:

     prev | msg  | msg  ||"cont"
     CONT |PREFIX| CONT || char
    ------+------+------++------
     clear| clear| clear||  '-'
     clear| clear|  set ||  'c'
     clear|  set | clear||  '-'
     clear|  set |  set ||  'c'
      set | clear| clear||  '+'
      set |  set |  set ||  '+'
      set |  set | clear||  '-'
      set |  set |  set ||  '+'      <-- should be 'c'

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Petr Mladek July 17, 2014, 10:14 a.m. | #1
On Wed 2014-07-16 12:26:58, Alex Elder wrote:
> In devkmsg_read(), a variable "cont" holds a character that's used
> to indicate whether a given log line is a "continuation", that is,
> whether a log record should be merged with the one before or after
> it.  If a record should be merged with its successor (but not its
> predecessor) that character is 'c'.  And the line following such a
> 'c' log record is normally marked with a '+' to show it is continues
> its predecessor.  Any other cases are marked '-', indicating the
> log record stands on its own.
> 
> There is an exception.  If a log record is marked LOG_PREFIX, it
> indicates that this record represents a new log entry, implicitly
> terminating the predecessor--even if the predecessor would otherwise
> have been continued.  So a record marked LOG_PREFIX (that is not
> also marked LOG_CONT) should have '-' for its "cont" variable.
> 
> The logic that determines this "continuation" character has a bug
> that gets that exceptional case wrong.
> 
> The specific case that produces the wrong result is when all of
> these conditions are non-zero:
>     user->prev & LOG_CONT
>     msg->flags & LOG_PREFIX
>     msg->flags & LOG_CONT
> The bug is that despite the message's LOG_PREFIX flag, the
> "cont" character is getting set to '+' rather than 'c'.
> 
> The problem is that the message's LOG_PREFIX flag is getting
> ignored if its LOG_CONT flag is also set.  Rearrange the logic
> here to produce the correct result.
> 
> The following table concisely defines the problem:
> 
>      prev | msg  | msg  ||"cont"
>      CONT |PREFIX| CONT || char
>     ------+------+------++------
>      clear| clear| clear||  '-'
>      clear| clear|  set ||  'c'
>      clear|  set | clear||  '-'
>      clear|  set |  set ||  'c'
>       set | clear| clear||  '+'
>       set |  set |  set ||  '+'
	       ^^^ typo, it should be:

       set |  clear |  set ||  '+'

>       set |  set | clear||  '-'
>       set |  set |  set ||  '+'      <-- should be 'c'
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  kernel/printk/printk.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 301ade3..9e9cf93 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -572,7 +572,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>  	struct printk_log *msg;
>  	u64 ts_usec;
>  	size_t i;
> -	char cont = '-';
> +	char cont;
>  	size_t len;
>  	ssize_t ret;
>  
> @@ -619,11 +619,12 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>  	 * better readable output. 'c' in the record flags mark the first
>  	 * fragment of a line, '+' the following.
>  	 */
> -	if (msg->flags & LOG_CONT && !(user->prev & LOG_CONT))
> -		cont = 'c';
> -	else if ((msg->flags & LOG_CONT) ||
> -		 ((user->prev & LOG_CONT) && !(msg->flags & LOG_PREFIX)))
> +	if (user->prev & LOG_CONT && !(msg->flags & LOG_PREFIX))
>  		cont = '+';
> +	else if (msg->flags & LOG_CONT)
> +		cont = 'c';
> +	else
> +		cont = '-';

Makes sense. Prefix should be always on a new line.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

Best Regards,
Petr

>  	len = sprintf(user->buf, "%u,%llu,%llu,%c;",
>  		      (msg->facility << 3) | msg->level,
> -- 
> 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/
Alex Elder July 17, 2014, 12:19 p.m. | #2
On 07/17/2014 05:14 AM, Petr Mládek wrote:
> On Wed 2014-07-16 12:26:58, Alex Elder wrote:
>> In devkmsg_read(), a variable "cont" holds a character that's used
>> to indicate whether a given log line is a "continuation", that is,
>> whether a log record should be merged with the one before or after
>> it.  If a record should be merged with its successor (but not its
>> predecessor) that character is 'c'.  And the line following such a
>> 'c' log record is normally marked with a '+' to show it is continues
>> its predecessor.  Any other cases are marked '-', indicating the
>> log record stands on its own.
>>
>> There is an exception.  If a log record is marked LOG_PREFIX, it
>> indicates that this record represents a new log entry, implicitly
>> terminating the predecessor--even if the predecessor would otherwise
>> have been continued.  So a record marked LOG_PREFIX (that is not
>> also marked LOG_CONT) should have '-' for its "cont" variable.
>>
>> The logic that determines this "continuation" character has a bug
>> that gets that exceptional case wrong.
>>
>> The specific case that produces the wrong result is when all of
>> these conditions are non-zero:
>>     user->prev & LOG_CONT
>>     msg->flags & LOG_PREFIX
>>     msg->flags & LOG_CONT
>> The bug is that despite the message's LOG_PREFIX flag, the
>> "cont" character is getting set to '+' rather than 'c'.
>>
>> The problem is that the message's LOG_PREFIX flag is getting
>> ignored if its LOG_CONT flag is also set.  Rearrange the logic
>> here to produce the correct result.
>>
>> The following table concisely defines the problem:
>>
>>      prev | msg  | msg  ||"cont"
>>      CONT |PREFIX| CONT || char
>>     ------+------+------++------
>>      clear| clear| clear||  '-'
>>      clear| clear|  set ||  'c'
>>      clear|  set | clear||  '-'
>>      clear|  set |  set ||  'c'
>>       set | clear| clear||  '+'
>>       set |  set |  set ||  '+'
> 	       ^^^ typo, it should be:

I will fix this, and it will show up when I re-post the series.

> 
>        set |  clear |  set ||  '+'
> 
>>       set |  set | clear||  '-'
>>       set |  set |  set ||  '+'      <-- should be 'c'
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  kernel/printk/printk.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 301ade3..9e9cf93 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -572,7 +572,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>>  	struct printk_log *msg;
>>  	u64 ts_usec;
>>  	size_t i;
>> -	char cont = '-';
>> +	char cont;
>>  	size_t len;
>>  	ssize_t ret;
>>  
>> @@ -619,11 +619,12 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>>  	 * better readable output. 'c' in the record flags mark the first
>>  	 * fragment of a line, '+' the following.
>>  	 */
>> -	if (msg->flags & LOG_CONT && !(user->prev & LOG_CONT))
>> -		cont = 'c';
>> -	else if ((msg->flags & LOG_CONT) ||
>> -		 ((user->prev & LOG_CONT) && !(msg->flags & LOG_PREFIX)))
>> +	if (user->prev & LOG_CONT && !(msg->flags & LOG_PREFIX))
>>  		cont = '+';
>> +	else if (msg->flags & LOG_CONT)
>> +		cont = 'c';
>> +	else
>> +		cont = '-';
> 
> Makes sense. Prefix should be always on a new line.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.cz>

Yay!!!  Thanks.

					-Alex

> 
> Best Regards,
> Petr
> 
>>  	len = sprintf(user->buf, "%u,%llu,%llu,%c;",
>>  		      (msg->facility << 3) | msg->level,
>> -- 
>> 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/

Patch hide | download patch | download mbox

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 301ade3..9e9cf93 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -572,7 +572,7 @@  static ssize_t devkmsg_read(struct file *file, char __user *buf,
 	struct printk_log *msg;
 	u64 ts_usec;
 	size_t i;
-	char cont = '-';
+	char cont;
 	size_t len;
 	ssize_t ret;
 
@@ -619,11 +619,12 @@  static ssize_t devkmsg_read(struct file *file, char __user *buf,
 	 * better readable output. 'c' in the record flags mark the first
 	 * fragment of a line, '+' the following.
 	 */
-	if (msg->flags & LOG_CONT && !(user->prev & LOG_CONT))
-		cont = 'c';
-	else if ((msg->flags & LOG_CONT) ||
-		 ((user->prev & LOG_CONT) && !(msg->flags & LOG_PREFIX)))
+	if (user->prev & LOG_CONT && !(msg->flags & LOG_PREFIX))
 		cont = '+';
+	else if (msg->flags & LOG_CONT)
+		cont = 'c';
+	else
+		cont = '-';
 
 	len = sprintf(user->buf, "%u,%llu,%llu,%c;",
 		      (msg->facility << 3) | msg->level,