Re: [PATCH v4 6/7] printk: insert newline for truncated records

[Alex Elder]

On 07/21/2014 06:57 AM, Petr Mládek wrote:
> On Fri 2014-07-18 16:28:04, Alex Elder wrote:
>> If a log record has LOG_PREFIX set, its predecessor record should be
>> terminated if it was marked LOG_CONT.
>>
>> In devkmsg_read(), this condition was being ignored, which would
>> lead to such records showing up combined when reading /dev/kmsg.
>> Fix this oversight.
>>
>> We should similarly insert a newline in msg_print_text() in this
>> case, to avoid formatted records getting merged.
>>
>> Suggested-by: Petr Mládek <pmladek@xxxxxxx>
>> Signed-off-by: Alex Elder <elder@xxxxxxxxxx>
>> ---
>>  kernel/printk/printk.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index e9f0632..a5ad81c 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -575,6 +575,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>>  	char cont;
>>  	size_t len;
>>  	ssize_t ret;
>> +	bool insert_newline;
>>  
>>  	if (!user)
>>  		return -EBADF;
>> @@ -626,7 +627,10 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>>  	else
>>  		cont = '-';
>>  
>> -	len = sprintf(user->buf, "%u,%llu,%llu,%c;",
>> +	/* Insert a newline if the previous line was not terminated properly */
>> +	insert_newline = (user->prev & LOG_CONT) && (msg->flags & LOG_PREFIX);
>> +	len = sprintf(user->buf, "%s%u,%llu,%llu,%c;",
>> +		      insert_newline ? "\n" : "",
>>  		      (msg->facility << 3) | msg->level,
>>  		      user->seq, ts_usec, cont);
>>  	user->prev = msg->flags;
>> @@ -999,10 +1003,12 @@ static size_t msg_print_text(const struct printk_log *msg, enum log_flags prev,
>>  {
>>  	const char *text = log_text(msg);
>>  	size_t text_size = msg->text_len;
>> +	size_t len = 0;
>> +	bool insert_newline;
>>  	bool prefix = true;
>>  	bool newline = true;
>> -	size_t len = 0;
>>  
>> +	insert_newline = (prev & LOG_CONT) && (msg->flags & LOG_PREFIX);
>>  	if ((prev & LOG_CONT) && !(msg->flags & LOG_PREFIX))
>>  		prefix = false;
>>  
>> @@ -1023,9 +1029,13 @@ static size_t msg_print_text(const struct printk_log *msg, enum log_flags prev,
>>  
>>  		if (buf) {
>>  			if (print_prefix(msg, syslog, NULL) +
>> -			    text_len + 1 >= size - len)
>> +			    text_len + 2 >= size - len)
> 
> It counts the '\n' even when it is not used.
> I think that it is even wrong that it calculates prefix when it is not used.

That's true, and I have yet another un-posted patch that
addresses this problem (well the second one).  I am not
going to fix this problem in this patch, but the fix is
coming.

Now that you're looking at the code I'm touching, you're
seeing the same things I did...

I think I'll start posting that series later today or
tomorrow.   I just hate to get too far ahead of myself.

>>  				break;
>>  
>> +			if (insert_newline) {
>> +				insert_newline = false;
>> +				buf[len++] = '\n';
>> +			}
>>  			if (prefix)
>>  				len += print_prefix(msg, syslog, buf + len);
>>  			memcpy(buf + len, text, text_len);
>> @@ -1034,6 +1044,8 @@ static size_t msg_print_text(const struct printk_log *msg, enum log_flags prev,
>>  				buf[len++] = '\n';
>>  		} else {
>>  			/* SYSLOG_ACTION_* buffer size only calculation */
>> +			if (insert_newline)
>> +				len++;
> 
> You forgot "insert_newline = false" here.

Yes you're right.  It's good that you're reviewing this.
(The patches I have not yet posted affect this area of
code, and should eliminate this block...)

>>  			if (prefix)
>>  				len += print_prefix(msg, syslog, NULL);
>>  			len += text_len;
> 
> It is just matter of personal style but I would suggest to do this
> before the do-while cycle:
> 
> 	/* Force newline if the previous text was not properly finished */
> 	if ((prev & LOG_CONT) && (msg->flags & LOG_PREFIX) && (len < size)) {
> 		if (buf)
> 			buf[len++] = '\n';
> 		else
> 			len++;
> 	}
> 
> IMHO, it is more clear. The do-while cycle already is complex enough.

I agree with this.  It's a one-time thing and doesn't belong in
the loop.  When you suggested inserting the newline I think I
didn't think it through completely.  I will do this.

> BTW: This is relared to the first patch. I would either patch all
> three locations in one patch or better split it into three patches.

I am keeping the first patch separate from this one.  I think
the first is related (in that we improve readability by inserting
some newlines) but it's really addressing a different problem.

Meanwhile, this patch is addressing essentially the same problem
in two spots, so I'd like to keep these together rather than
splitting it in two.

I will move this patch earlier in the series, however, making
it follow patch 1.

I may be misunderstanding what you mean though.  Is what I
propose OK with you?

Thank you.

					-Alex


> 
> Best Regards,
> Petr
> 

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