lkml.org 
[lkml]   [2015]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 04/16] printk: implement support for extended console drivers
On Mon 2015-04-27 17:09:22, Tejun Heo wrote:
> Hello, Petr.
>
> Sorry about the delay.

np

> On Mon, Apr 20, 2015 at 05:43:07PM +0200, Petr Mladek wrote:
> > I was afraid that there might be a potential buffer overflow because
> > the user-provided dict need not be limited by '\0'. But LOG_DICT_META
> > is used only with the internal data that are safe. We might document
> > this as well.
> >
> > BTW: Do you want to print the internal dict when calling this function
> > in devkmsg_read()?
>
> No, plesae see below.

I see, it is not printed there because the dict_len is zero for the
internal info.

> > > + scnprintf(fragid_buf, sizeof(fragid_buf),
> > > + ",fragid=%llu", fragid);
> > > + return scnprintf(buf, size, "%u,%llu,%llu,%c%s;",
> > > + (msg->facility << 3) | msg->level, seq, ts_usec, cont,
> > > + fragid_buf);
> >
> > The above comment suggests that "cont" and "fragid_buf" are delimited
> > by a comma but it is not used here. Is it by intention.
>
> Hmm... how is it not? The fragid printf has preceding comma.

I see it now.

> > > + dict_len = scnprintf(dict_buf, sizeof(dict_buf), "FRAGID=%llu",
> > > + cont.fragid);
> > > + log_store(cont.facility, cont.level,
> > > + flags | LOG_NOCONS | LOG_DICT_META,
> > > + cont.ts_nsec, dict_buf, dict_len, cont.buf, cont.len);
> >
> > I wonder if it would make sense to restart fragid here. Another line
> > will get distinguished by "seqnum".
>
> That'd assume that there can only ever be a single continuation
> buffer, which is true now but it's possible that we may want to make
> it per-cpu in the future.

I am not sure if any more complications will get accepted. Anyway,
each CPU should print an independent message. Therefore each pre-CPU
cont buffer would be printed on separate line and would get another
seqnum. We could have per-CPU fragid counter.


> > Sigh, the whole mess is caused by the fact that we could not extend
> > struct printk_log easily. It would be much better if we could put
> > fragid there. I finally understand why you need to reuse the dict.
>
> I've been thinking about it and using dict area for internal metadata
> is indeed quite messy. I think a better way to do it is declaring
> dict_len as a union w/ fragid. This'd limit the fragid to 16bit but
> that should be more than enough and we can do away with the string
> formatting and reading back.

Nice idea. I like it much more.


> > Another solution would be to allow to disable the continuous buffer
> > via some boot option or /sys/kernel/debug entry if you want to debug
> > such a problem and have problem with the partial flushing.
>
> It isn't really about debugging partial flushing itself but rather
> always being able to push out the messages before the printk
> invocation finishes.

I did not mean cont-buffer debugging. I meant debugging with
netconsole.

Note that cont buffer is only a memory optimization. You could put
every text snippet into the ring buffer and console immediately.
You need to set LOG_CONT instead of LOG_NEWLINE flag. This is
already used when the line is too long for the cont buffer or
when there is printed a message from another cpu in the middle
of a continuing message.


Best Regards,
Petr


\
 
 \ /
  Last update: 2015-04-28 12:21    [W:0.115 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site