Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 7/7] printk: Handle dropped message smarter | Date | Wed, 07 Dec 2022 18:04:56 +0106 |
| |
On 2022-12-07, Petr Mladek <pmladek@suse.com> wrote: >> + /* Print it into ext_text, which is unused. */ >> + len = snprintf(ext_text, DROPPED_TEXT_MAX, >> + "** %lu printk messages dropped **\n", dropped); >> + > > I would feel better if we check here that the text fits into the rest > of the buffer. > > if (WARN_ON_ONCE(len + cmsg->outbuf_len < sizeof(cbufs->ext_text))) > return; > > I know that it is kind-of guaranteed by the above compilation check > of the *_MAX values. But there might be a bug and cmsg->outbuf_len > might contains a garbage.
OK.
>> + /* >> + * Append the record text to the dropped message so that it >> + * goes out with one write. >> + */ >> + memcpy(ext_text + len, &cbufs->text[0], cmsg->outbuf_len); >> + >> + /* Update the output buffer descriptor. */ >> + cmsg->outbuf = ext_text; >> + cmsg->outbuf_len += len; > > I still think that it would be better to rename the buffers in > struct console_message and avoid the switches of the purpose > of the two buffers. > > We could print the message about dropped text into a local buffer > on stack. IMHO, 64 bytes are acceptable. And we could insert it > into the outbuf by shuffling the existing text. Something like: > > static void msg_print_dropped(struct console_message *cmsg, > unsinged long dropped) > { > char dropped_msg[DROPPED_TEXT_MAX]; > int dropped_len; > > if (!con->dropped) > return 0; > > /* Print it into ext_text, which is unused. */ > dropped_len = snprintf(dropped_msg, sizeof(dropped_msg), > "** %lu printk messages dropped **\n", con->dropped); > > /* > * The buffer might already be full only where the message consist > * of many very short lines. It is not much realistic. > */ > if (cmsg->outbuf_len + dropped_len + 1 > sizeof(cmsg->outbuf)) { > /* Should never happen. */
This certainly can happen. @text is size CONSOLE_LOG_MAX, which is LOG_LINE_MAX+PREFIX_MAX. So a totally legal formatted message of length LOG_LINE_MAX-1 and a prefix will suddenly become truncated.
> if (WARN_ON_ONCE(dropped_len + 1 > sizeof(cmsg->outbuf))) > return; > > /* Trunk the message like in record_print_text() */ > cmsg->outbuf_len = sizeof(cmsg->outbuf) - dropped_len; > cmsg->outbuf[cmsg->outbuf_len] = '\0'; > } > > memmove(cmsg->outbuf + dropped_len, cmsg->outbuf, cmsg->outbuf_len + 1); > memcpy(cmsg->outbuf, dropped_msg, dropped_len); > }
I do not like the idea of increasing stack usage, possibly cutting off messages, and performing extra memory operations all because of some variable naming. There is literally a larger unused buffer just sitting there.
I want struct console_buffers to be a black box of working buffers used to process the different types of messages. console_get_next_message() is the only function that should go inside that black box because it is responsible for creating the actual message.
The caller does not care what is in the black box or how those internal working buffers are named. The caller just cares about cmsg->outbuf and cmsg->outbuf_len, which will point to the data that needs to be written out.
For v3 I would like to try my approach one more time. I will give the internal buffers new names so as not to mislead their roles. I will clearly document the black box nature of struct console_buffers.
John Ogness
| |