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

[Chris Down]

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