Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

Chris Down
2022-09-05T14:45:03Z

Hi Petr,

Thanks a lot for looking this over!

Petr Mladek writes:
>I think that the function does not work as expected.
>
>> + bool seen_serial_opts = false;
>> + char *key;
>> +
>> + while ((key = strsep(&options, ",")) != NULL) {
>> + char *value;
>
>strsep() replaces ',' with '\0'.
>
>> +
>> + value = strchr(key, ':');
>> + if (value)
>> + *(value++) = '\0';
>
>This replaces ':' with '\0'.
>
>> +
>> + if (!seen_serial_opts && isdigit(key[0]) && !value) {
>
>This catches the classic options in the format "bbbbpnf".
>
>> + seen_serial_opts = true;
>> + c->options = key;
>> + continue;
>> + }
>> +
>> + pr_err("ignoring invalid console option: '%s%s%s'\n", key,
>> + value ? ":" : "", value ?: "");
>
>IMHO, this would warn even about "io", "mmio", ... that are used by:

Oh dear, I should have known it won't be that simple :-D

>
> console=uart[8250],io,<addr>[,options]
> console=uart[8250],mmio,<addr>[,options]
>
>Warning: I am not completely sure about this. It seems that
> this format is handled by univ8250_console_match().
>
> IMHO, the "8250" is taken as "idx" in console_setup().
> And "idx" parameter is ignored by univ8250_console_match().
> This probably explains why "8250" is optional in console=
> parameter.
>
>> + }
>> +}
>
>Sigh, the parsing of console= parameter is really hacky. Very old code.
>The name and idx is handled in console_setup(). The rest
>is handled by particular drivers via "options".
>
>This patch makes it even more tricky. It tries to handle some
>*options in add_preferred_console(). But it replaces all ','
>and ':' by '\0' so that drivers do not see the original *options
>any longer.

Other than the mmio/io stuff, I think it works properly, right? Maybe I'm
missing something? :-)

Here's a userspace test for the parser that seems to work.
parse_console_cmdline_options() should also ignore empty options instead of
warning, but it still functions correctly in that case, it's just noisy right
now.

---

/* Only change is pr_err to fprintf */

#define _DEFAULT_SOURCE

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <ctype.h>
#include <stdlib.h>

#define clamp(a, b, c) (a)
#define CON_LOGLEVEL 128

struct console_cmdline {
char *options;
int level;
short flags;
};

static void parse_console_cmdline_options(struct console_cmdline *c,
char *options)
{
bool seen_serial_opts = false;
char *key;

while ((key = strsep(&options, ",")) != NULL) {
char *value;

value = strchr(key, ':');
if (value)
*(value++) = '\0';

if (strcmp(key, "loglevel") == 0 && value &&
isdigit(value[0]) && strlen(value) == 1) {
c->level = clamp(value[0] - '0', LOGLEVEL_EMERG,
LOGLEVEL_DEBUG + 1);
c->flags |= CON_LOGLEVEL;
continue;
}

if (!seen_serial_opts && isdigit(key[0]) && !value) {
seen_serial_opts = true;
c->options = key;
continue;
}

fprintf(stderr,
"ignoring invalid console option: '%s%s%s'\n", key,
value ? ":" : "", value ?: "");
}
}

int main(int argc, char *argv[])
{
struct console_cmdline con = { 0 };

if (argc != 2)
return EXIT_FAILURE;

parse_console_cmdline_options(&con, argv[1]);
printf("options: %s\n", con.options);
printf("level: %d\n", con.level);
}

---

% make CFLAGS='-Wall -Wextra -Werror' loglevel
cc -Wall -Wextra -Werror loglevel.c -o loglevel
% ./loglevel 9600n8
options: 9600n8
level: 0
level set: 0
% ./loglevel 9600n8,loglevel:3
options: 9600n8
level: 3
level set: 1
% ./loglevel 9600n8,loglevel:123
ignoring invalid console option: 'loglevel:123'
options: 9600n8
level: 0
level set: 0
% ./loglevel 9600n8,loglevel:3,foo:bar
ignoring invalid console option: 'foo:bar'
options: 9600n8
level: 3
level set: 1
% ./loglevel 9600n8,loglevel
ignoring invalid console option: 'loglevel'
options: 9600n8
level: 0
level set: 0
% ./loglevel loglevel
ignoring invalid console option: 'loglevel'
options: (null)
level: 0
level set: 0
% ./loglevel loglevel:7
options: (null)
level: 7
level set: 1

---

Seems to work ok as far as I can tell, maybe I've misunderstood your concern?
Or maybe your concern is just about the mmio/io case where the driver wants
that as part of the options?

>I thought a lot how to do it a clean way. IMHO, it would be great to
>parse everything at a single place but it might require updating
>all drivers. I am not sure if it is worth it.
>
>So, I suggest to do it another way. We could implement a generic
>function to find in the new key[:value] format. It would check
>if the given option (key) exists and read the optional value.
>
>The optional value would allow to define another new options
>that would not need any value, e.g. "kthread" or "atomic" that
>might be used in the upcoming code that allows to offload
>console handling to kthreads.

This could also work, and avoids the current null pointer shoving. It also can
make the ordering less strict, which seems like a good thing.

I will think about it some more. I'm curious about your comments on the above
test still though.

Thanks a lot for the detailed review and ideas!

Chris