Re: [PATCH v5] printk: Add line-buffered printk() API.

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2018/11/01 23:13, Petr Mladek wrote:
> On Wed 2018-10-24 19:11:10, Tetsuo Handa wrote:
>> After this patch, we will consider how we can add context identifier to
>> each line of printk() output (so that we can group multiple lines into
>> one block when parsing). Therefore, if converting to this API does not
>> fit for some reason, you might be able to consider converting to multiple
>> printk() calls which end with '\n'.
> 
> The buffered printk API is for continuous lines. It is more
> complicated than a simple printk. You need to get, use, and put
> the buffer. It might be acceptable for continuous lines that
> should be rare and the related calls typically located in a single
> function.
> 
> I prefer to solve the related lines on another level, for example,
> by storing/showing PID+context_mask for each printed line. This
> way it would work transparently even for normal printk().

Yes. I'm expecting that identifier part is added by printk()
rather than by this line buffering API.



>> +	/*
>> +	 * Skip KERN_CONT here based on an assumption that KERN_CONT will be
>> +	 * given via "fmt" argument when KERN_CONT is given.
>> +	 */
>> +	fmt_offset = (printk_get_level(fmt) == 'c') ? 2 : 0;
>> + retry:
>> +	va_copy(tmp_args, args);
>> +	r = vsnprintf(ptr->buf + ptr->used, sizeof(ptr->buf) - ptr->used,
>> +		      fmt + fmt_offset, tmp_args);
>> +	va_end(tmp_args);
>> +	if (r + ptr->used < sizeof(ptr->buf)) {
>> +		ptr->used += r;
>> +		/* Flush already completed lines if any. */
>> +		for (r = ptr->used - 1; r >= 0; r--) {
>> +			if (ptr->buf[r] != '\n')
>> +				continue;
> 
> I thought about using strrchr(). But this is more effective
> because we know the length of the string. It might deserve
> a comment though.

At first, I tried to use memrchr().

>> +			break;
>> +		}
>> +		return r;
> 
> We need to use another variable in the for-cycle. Otherwise, we would
> not return the number of printed characters here.

But since memrchr() is not available, I forgot to introduce another
variable when rewriting this loop...



>> +bool flush_printk_buffer(struct printk_buffer *ptr)
>> +{
>> +	if (!ptr || !ptr->used)
>> +		return false;
>> +	/* vprintk_buffered() keeps 0 <= ptr->used < sizeof(ptr->buf) true. */
>> +	ptr->buf[ptr->used] = '\0';
> 
> We do not need this when there is always the trailing '\0' in
> non-empty buffer. It looks more sane to me.

We need this when called from vprintk_buffered(), for vsnprintf()
has overwritten ptr->buf[ptr->used] with non-'\0' byte.

>> +	printk("%s", ptr->buf);
>> +	ptr->used = 0;
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(flush_printk_buffer);



> We are getting close. Please, split the debugging stuff into separate
> patch.

Too many comments on debugging stuff. I will for now drop debugging stuff
 from next version. We can add debugging stuff after core patch is merged.

> 
> Also it would be great to do add a sample conversion from pr_cont() to
> this API in another separate patch.

OK. I will add two example users in next version.