Messages in this thread | | | From | John Ogness <> | Subject | Re: vprintk_store: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock | Date | Sun, 06 Dec 2020 23:36:53 +0106 |
| |
On 2020-12-04, Petr Mladek <pmladek@suse.com> wrote: >> + /* Strip kernel syslog prefix. */ > > Syslog actually uses: <level> format. We are skipping log level > and control flags here.
OK.
>> + if (facility == 0) { >> + while (text_len >= 2 && printk_get_level(text)) { >> + text_len -= 2; >> + text += 2; >> + } > > We should avoid two completely different approaches > that handle printk_level prefix. > > One solution is to implement something like: > > static char *parse_prefix(text, &level, &flags) > > That would return pointer to the text after the prefix. > And fill level and flags only when non-NULL pointers are passed.
OK.
> Another solution would be to pass this information from > vprintk_store(). The prefix has already been parsed > after all.
Well, there is a vscnprintf() that happens in between and I don't think we should trust the parsed offset from the first vsnprintf().
>> + >> + if (text != orig_text) >> + memmove(orig_text, text, text_len); >> + } > > We should clear the freed space to make the ring buffer as > human readable as possible when someone just dumps the memory.
Data blocks are currently padded and that padding is not cleared. So it is already not perfectly human readable on a raw dump.
> Sigh, I have to admit that I missed the problem with prefix and > trailing '\n' when I suggested to avoid the temporary buffers. > This memmove() and the space wasting is pity. > > Well, it is typically 3 bytes per message. And the copying would > be necessary even with the temporary buffer. So, I am less convinced > but I would still try to avoid the temporary buffers for now.
Agreed. I think this approach is better than the temporary buffers I previously used. Also, if we add a trimming feature to the ringbuffer, it will keep the ringbuffer mostly clean anyway. Something like this:
prb_rec_init_wr(&r, text_len); prb_reserve(&e, prb, &r); text_len = printk_sprint(&r.text_buf[0], text_len, ...); r.info->text_len = text_len; prb_trim_rec(&e, &r); <--- try to reduce datablock size to @text_len prb_commit(&e);
I see no urgency to add such a feature. But I think we should keep it on our radar.
John Ogness
| |