From: Petr Mladek <pmladek@suse.com>
To: Chris Down <chris@chrisdown.name>
Cc: linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Steven Rostedt <rostedt@goodmis.org>,
John Ogness <john.ogness@linutronix.de>,
Geert Uytterhoeven <geert@linux-m68k.org>,
kernel-team@fb.com
Subject: Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options
Date: Tue, 18 Apr 2023 15:41:31 +0200 [thread overview]
Message-ID: <ZD6eC9UBxwg-sG5_@alley> (raw)
In-Reply-To: <ZD1f-1JjpihR1djd@chrisdown.name>
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
next prev parent reply other threads:[~2023-04-18 13:41 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-20 17:48 [PATCH v3 0/2] printk: console: Per-console loglevels Chris Down
2022-07-20 17:48 ` [PATCH v3 1/2] printk: console: Create console= parser that supports named options Chris Down
2022-08-31 10:13 ` Petr Mladek
2022-09-05 14:43 ` Chris Down
2022-09-05 14:45 ` Chris Down
2022-09-22 15:17 ` Petr Mladek
2023-04-17 15:04 ` Chris Down
2023-04-17 15:08 ` Chris Down
2023-04-18 13:43 ` Petr Mladek
2023-04-18 13:41 ` Petr Mladek [this message]
2023-04-19 0:31 ` Chris Down
2022-07-20 17:48 ` [PATCH v3 2/2] printk: console: Support console-specific loglevels Chris Down
2022-07-21 9:50 ` kernel test robot
2022-07-21 16:20 ` Chris Down
2022-07-21 16:16 ` kernel test robot
2022-07-21 16:29 ` Chris Down
2022-09-01 9:35 ` Petr Mladek
2022-09-05 14:07 ` Chris Down
2022-09-05 15:12 ` Petr Mladek
2022-07-20 18:22 ` [PATCH v3 0/2] printk: console: Per-console loglevels John Ogness
2022-07-20 18:29 ` Chris Down
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZD6eC9UBxwg-sG5_@alley \
--to=pmladek@suse.com \
--cc=chris@chrisdown.name \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=john.ogness@linutronix.de \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=senozhatsky@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).