Messages in this thread | | | Date | Fri, 7 Oct 2022 17:15:32 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH printk 15/18] printk: Add struct cons_text_buf |
| |
On Sat 2022-09-24 02:10:51, John Ogness wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Create a data structure to replace the open coded separate buffers for > regular and extended formatting. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > include/linux/console.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/linux/console.h b/include/linux/console.h > index 8ec24fe097d3..05c7325e98f9 100644 > --- a/include/linux/console.h > +++ b/include/linux/console.h > @@ -173,6 +173,20 @@ enum cons_flags { > CON_EXTENDED = BIT(6), > }; > > +/** > + * struct cons_text_buf - console output text buffer > + * @ext_text: Buffer for extended log format text > + * @dropped_text: Buffer for dropped text > + * @text: Buffer for ringbuffer text > + */ > +struct cons_text_buf {
Sigh, I feel bad to nit-pick about this. It seems that you have used "cons" everywhere in the new API so any change might be painful. But I personally find really handful when an API is predictable and consistent.
I see that "cons" has already been used few times in tty subsystem, especially tty/vt and tty/hvc.
But I do not see any single "cons_" under kernel/printk/ before this patchset. Either "console_" or "con_" prefix was used everywhere, including CON_XXX flags.
Is there any change to change this to either "console_" or "con_", please? Or is there any particular reason why this new API should be distinguished by the new prefix?
> + union { > + char ext_text[CONSOLE_EXT_LOG_MAX]; > + char dropped_text[DROPPED_TEXT_MAX]; > + }; > + char text[CONSOLE_LOG_MAX];
We should explain in the commit message why we need the separate ext_text buffer and why it can be shared with dropped_text buffer. Something like:
<proposal> Create a data structure to replace the open coded separate buffers for regular and extended formatting.
Separate @ext_text buffer is needed because info_print_ext_header() and msg_print_ext_body() are not able to add the needed extra information inplace.
@ext_text and @dropped_text buffer can be shared because they are never used at the same time. </proposal>
Also I think about using pointers instead of the hard-coded buffer size. For example, there is no need to have the big ext_text buffer in the kthread when the related console does not allow to allocated the extended text. There is actually only one console that has this enabled.
I mean something like:
struct cons_text_buf { char *text; char *ext_text; char *dropped_text;
unsigned int text_size; unsigned int ext_text_size; unsigned int dropped_text_size; }
We might create a helper to define static buffer:
#define DEFINE_CONS_TEXT_BUF(name) \ static char _##name##_text[CONSOLE_LOG_MAX]; \ static char _##name##_ext_text[CONSOLE_EXT_LOG_MAX]; \ static struct const_text_buf name = { \ .text = _##name##_text, \ .ext_text = _##name##_ext_text, \ .dropped_text = _##name##_ext_text, \ \ .text_size = CONSOLE_LOG_MAX; \ .ext_text_size = CONSOLE_LOG_MAX; \ .dropped_text_size = DROPPED_TEXT_MAX; \ };
Another advantage would be that it looks like a more safe way to pass the buffer size. The existing code hardcodes CONSOLE_LOG_MAX and CONSOLE_EXT_LOG_MAX everywhere. And it is less obvious that the buffer and size fits together. Especially that the names do not match (text vs. LOG_MAX and ext_text vs. EXT_LOG_MAX).
Well, this might be out of scope of this patchset. I do not resist on it. We might do this later.
Best Regards, Petr
| |