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

From: Petr Mladek
Date: Tue Apr 18 2023 - 09:41:44 EST


On Mon 2023-04-17 16:04:27, Chris Down wrote:
> (To others on this thread wondering about this patchset, Petr and I have had
> some discussions offlist about v4 and it should be up soon.)
>
> Petr Mladek writes:
> > 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.
>
> Any thoughts on something simple like this that takes advantage of
> memmove()? This should overcome the mmio/io concerns, and it's fairly
> simple.
>
> ---
>
> static bool find_and_remove_console_option(char *buf, size_t size,
> const char *wanted, char *options)

Nit: I would change the ordering of the parameters. The above uses
the semantic of copy functions (desc, src). But this function
is more about searching or reading. I would use semantic like
strchr() or read() (where, what, buf).

Also I would use the key:value names.

Something like:

static bool
find_and_remove_console_option(char *options, const char
char *val_buf, size_t val_buf_size)

> {
> bool found = false, first = true;
> char *item, *opt = options;

Nit: I would rename these:

+ item -> option: the function is searching for an option that
has the format value:key.

+ opt -> next: make it more clear that it points behind the
currently proceed option (string token).

> while ((item = strsep(&opt, ","))) {
> char *key = item, *value;
>
> value = strchr(item, ':');
> if (value)
> *(value++) = '\0';
>
> if (strcmp(key, wanted) == 0) {
> found = true;
> if (value) {
> if (strlen(value) > size - 1) {
> pr_warn("Can't copy console option value for %s:%s: not enough space (%zu)\n",
> key, value, size);
> found = false;
> } else {
> strscpy(buf, value, size);
> }
> } else
> *buf = '\0';
> }
>
> if (!found && opt)
> *(opt - 1) = ',';
> if (!found && value)
> *(value - 1) = ':';
> if (!first)
> *(item - 1) = ',';

This last assigned should not be needed. The above code replaced
at max one ',' and one ':'. It should be enough to restore
just the two.

> if (found)
> break;
>
> first = false;
> }
>
> if (found) {
> if (opt)
> memmove(item, opt, strlen(opt) + 1);
> else if (first)
> *item = '\0';
> else
> *--item = '\0';
> }
>
> return found;
> }

Otherwise, it looks correct.

Note: I though about using strnchr() and strncmp() instead of
replacing/restoring the two delimiters by '\0'.
But the code looks more hairy in the end.

Just for record, here is my attempt:

static bool
find_and_remove_console_option(char *options, const char *key,
char *val_buf, size_t val_buf_size)
{
char *start, *next, *val;
int option_len, key_len, found_key_len;
bool found = false;

if (val_buf && val_buf_size)
*val_buf = '\0';

key_len = strlen(key);
next = options;
do {
start = next;
next = strchr(start, ',');
if (next) {
option_len = next - start;
next++;
} else {
option_len = strlen(start);
}

val = strnchr(start, option_len, ':');
if (val) {
found_key_len = val - start;
val++;
} else {
found_key_len = option_len;
}

if (key_len != found_key_len)
continue;

if (!strncmp(start, key, key_len)) {
found = true;
break;
}
} while (next);

if (found && val) {
int val_len = option_len - key_len - 1;

if (!val_buf || val_buf_size < val_len + 1) {
pr_err("Can't copy value for the console option key: %s:%.*s\n",
key, val_len, val);
return false;
}

strscpy(val_buf, val, val_len + 1);
}

/* Remove the found value[:key][,] */
if (found) {
if (next)
memmove(start, next, strlen(next) + 1);
else if (start == options)
*options = '\0';
else
*(start - 1) = '\0';
}

return found;
}

Best Regards,
Petr