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

From: Alex Elder
Date: Mon Jul 21 2014 - 08:32:17 EST


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/