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.
