lkml.org 
[lkml]   [2022]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-10-07 17:16    [W:0.377 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site