On Sun 2020-12-06 23:36:53, John Ogness wrote: > On 2020-12-04, Petr Mladek <[email protected]> wrote: > >> + 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().
Good point! > >> + > >> + 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. It would be nice to clean up the padding as well. But it is a cosmetic improvement that might be done anytime later. > > 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. Another motivation is that it allows to simply handle recursion/nesting. Othrewise, we would need temporary buffers for each allowed recursion level or some tricky code. > 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. Yup. I thought about it as well. I agree that it is not a priority. Best Regards, Petr
