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

[Alex Elder <elder@linaro.org>]

On 07/21/2014 08:57 AM, Petr Mládek wrote:
> On Mon 2014-07-21 07:32:18, Alex Elder wrote:
>> 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.

. . .

>>>> @@ -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.
> 
> I suggest to always wait at least 24 hours before sending another
> version. I think that it is very hard for others to follow if there
> are too many versions in the wild and the code is too changing.
> This is why you get comments basically only from me.

Well, what I'm talking about would be a new series.  But I'll
wait I guess.

FYI, here's what I have already posted:
- This series (v5 has now been posted)
- A "printk: more log flag simplification" series that
  follows this one (had problems; will be reposted as
  a truncated set)

And here are some things I have queued, but have not yet
posted for review:
- A patch that corrects the size calculation issue you
  also noticed in msg_print_text().
- A series that I'm still verifying, which avoids formatting
  all printk() messages repeatedly.

I will try to avoid having too many things out at once.

> Also a night usually helps to sort ideas and go the right direction.
> In fact, I was too fast myself as well. I am going to comment only one
> version per-day from now on :-)

Oh, all right.  :)

>>>>  				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);

. . .

>> 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 do not have strong opinion here.

I will keep it as I posted in v5.  I moved this patch
earlier in the series, but did not combine it with the
first one.

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

Thanks for all your timely reviews, Petr.

					-Alex