v3:
- Update to work with John's kthread patches
- Remove force_console_loglevel, now we only have global and local levels
- Remove minimum_console_loglevel control and document how to change it
- The minimum loglevel is now only honoured on setting global/local level
- Add ignore_per_console_loglevel
- Return -EINVAL if trying to set below minimum console level
- Add parser for named console= options
- Fix docs around ignore_loglevel: it can be changed at runtime
- Fix ordering in "in order of authority" docs
- Remove duplicated default_console_loglevel doc
- Only warn once on syslog() use
v2:
- Dynamically allocate struct device*
- Document sysfs attributes in Documentation/ABI/
- Use sysfs_emit() instead of sprintf() in dev sysfs files
- Remove WARN_ON() for device_add/IS_ERR(console_class)
- Remove "soon" comment for kernel.printk
- Fix !CONFIG_PRINTK build
- Fix device_unregister() NULL dereference if called before class setup
- Add new documentation to MAINTAINERS
Chris Down (2):
printk: console: Create console= parser that supports named options
printk: console: Support console-specific loglevels
Documentation/ABI/testing/sysfs-class-console | 43 +++
.../admin-guide/kernel-parameters.txt | 28 +-
.../admin-guide/per-console-loglevel.rst | 92 ++++++
Documentation/admin-guide/serial-console.rst | 17 +-
Documentation/core-api/printk-basics.rst | 35 +-
Documentation/networking/netconsole.rst | 17 +
MAINTAINERS | 3 +
include/linux/console.h | 24 ++
kernel/printk/console_cmdline.h | 2 +
kernel/printk/printk.c | 311 +++++++++++++++++-
kernel/printk/sysctl.c | 64 +++-
11 files changed, 599 insertions(+), 37 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-console
create mode 100644 Documentation/admin-guide/per-console-loglevel.rst
base-commit: 9d882352bac8f2ff3753d691e2dc65fcaf738729
--
2.37.1
This will be used in the next patch, and for future changes like the
proposed sync/nothread named options. This avoids having to use sigils
like "/" to get different semantic meaning for different parts of the
option string, and helps to make things more human readable and
easily extensible.
There are no functional changes for existing console= users.
Signed-off-by: Chris Down <[email protected]>
---
kernel/printk/printk.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b49c6ff6dca0..6094f773ad4a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2368,6 +2368,30 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
console_set_on_cmdline = 1;
}
+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 (!seen_serial_opts && isdigit(key[0]) && !value) {
+ seen_serial_opts = true;
+ c->options = key;
+ continue;
+ }
+
+ pr_err("ignoring invalid console option: '%s%s%s'\n", key,
+ value ? ":" : "", value ?: "");
+ }
+}
+
static int __add_preferred_console(char *name, int idx, char *options,
char *brl_options, bool user_specified)
{
@@ -2393,7 +2417,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
if (!brl_options)
preferred_console = i;
strlcpy(c->name, name, sizeof(c->name));
- c->options = options;
+ parse_console_cmdline_options(c, options);
set_user_specified(c, user_specified);
braille_set_options(c, brl_options);
--
2.37.1
On 2022-07-20, Chris Down <[email protected]> wrote:
> v3:
>
> - Update to work with John's kthread patches
This will get a bit tricky, since I am also preparing a new kthread
series. But for now it is helpful to base your work on the previous
kthread work.
Thanks for your efforts on this!
John
John Ogness writes:
>On 2022-07-20, Chris Down <[email protected]> wrote:
>> v3:
>>
>> - Update to work with John's kthread patches
>
>This will get a bit tricky, since I am also preparing a new kthread
>series. But for now it is helpful to base your work on the previous
>kthread work.
No worries -- I don't think it should get too bad. If I recall correctly, the
only real changes were around handling suppress_message_printing in the new
model, and they were fairly minor.
>Thanks for your efforts on this!
Likewise on the kthread work! Just let me know if/when you want me to rebase.
:-)
On Wed 2022-07-20 18:48:11, Chris Down wrote:
> This will be used in the next patch, and for future changes like the
> proposed sync/nothread named options. This avoids having to use sigils
> like "/" to get different semantic meaning for different parts of the
> option string, and helps to make things more human readable and
> easily extensible.
>
> There are no functional changes for existing console= users.
>
> Signed-off-by: Chris Down <[email protected]>
> ---
> kernel/printk/printk.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b49c6ff6dca0..6094f773ad4a 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2368,6 +2368,30 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
> console_set_on_cmdline = 1;
> }
>
> +static void parse_console_cmdline_options(struct console_cmdline *c,
> + char *options)
> +{
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:
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.
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.
The function would look like:
/**
* find_and_remove_console_option() - find and remove particular option from
* console= parameter
*
* @options: pointer to the buffer with original "options"
* @key: option name to be found
* @buf: buffer for the found value, might be NULL
*
* Try to find "key[:value]" in the given @options string. Copy the value
* into the given @buf when any.
*
* Warning: The function is designed only to handle new generic console
* options. The found "key[:value]" is removed from the given
* @options string. The aim is to pass only the driver specific
* options to the legacy driver code.
*
* The new options might stay when all console drivers are able
* to handle it.
*
* The given @options buffer is modified to avoid allocations. It
* makes the life easier during early boot.
*/
bool find_and_remove_console_option(char *options, const char *key,
char *buf, int size)
{
bool found = false, copy_failed = false. trailing_comma = false;
char *opt, *val;
/* Find the option: key[:value] */
while (options && *options) {
opt = options;
options = strchr(options, ",");
if (options)
*options++ = '\0';
val = strchr(opt, ":");
if (val)
*val++ = '\0';
if (strcmp(opt, key) == 0) {
found = true;
break;
}
/* Not our key. Keep it in options. */
if (options) {
*(options - 1) = ',';
trailing_comma = 1;
}
if (val)
*(val - 1) = ':';
}
/* Copy the value if found. */
if (found && val) {
if (buf && size > strlen(val)) {
strscpy(buf, val, size);
} else {
pr_warn("Can't copy console= option value. Ignoring %s:%s\n",
opt, val);
copy_failed = true;
}
}
/* Remove the found option. */
if (found) {
if (options) {
strcpy(opt, options);
options = opt;
} else if (trailing_comma) {
/* Removing last option */
*(opt - 1) = '\0';
} else
*(opt) = '\0';
}
}
return found && !copy_failed;
}
then we could do something like:
void handle_per_console_loglevel_option(struct console_cmdline *c, char *options)
{
char buf[10];
int loglevel, ret;
if (!find_and_remove_console_option("options, "loglevel", buf, sizeof(buf)))
return;
if (kstrtoint(buf, 10, &loglevel)) {
pr_warn("Invalid console loglevel:%s\n", buf);
return;
}
if (loglevel < CONSOLE_LOGLEVEL_MIN || loglevel > CONSOLE_MOTORMOUTH) {
pr_warn("Console loglevel out of range: %d\n", loglevel);
return;
}
c->loglevel = loglevel;
c->flags |= CON_LOGLEVEL;
}
Note that the code is not even compile tested.
Any better ideas are highly appreciated.
Best Regards,
Petr
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
Chris Down writes:
>[code]
Er, forgot to update that for the one used in the test:
---
#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);
printf("level set: %d\n", !!(con.flags & CON_LOGLEVEL));
}
On Mon 2022-09-05 15:43:53, Chris Down wrote:
> 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? :-)
One important thing is to do not warn about mmio/io when they are
valid options.
Another important thing is to restore *options string. I mean to
revert all the added '\0' when they are not longer needed.
The *options buffer is passed as paramter to both con->match()
and con->setup() callbacks, see try_enable_preferred_console().
They must be able to see all the values.
Well, I would remove the newly added "logleve:X" when it is handled
in __add_preferred_console(). Otherwise, we would need to double
check all con->match() and con->setup() callbacks that they are
able to handle (ignore) this new option.
> 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.
> % 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?
Yes, the mmio/io stuff is the known problem.
My concern is that the function is destructive. It modifies the given
*options buffer that it later passed to another functions. It means
that it modifies the existing behavior.
I am afraid that you might just add an extra check to keep the
particular mmio/io parameters. But I am not sure if these are there only
special parameters used by console drivers.
We found about mmio/io parameters because they were mentioned in
Documentation/admin-guide/kernel-parameters.txt. But there might
be another parameters used by some exotic console that are documented
somewhere else.
I would prefer to do it the other (conservative) way around. I mean
that the new parser in __add_preferred_console() will only search
for the newly added ",loglevel:X" option and remove it from *options
buffer. Everything else should stay in the buffer so that it can
be parsed by the particular console drivers.
It actually already works this way. console_setup() searches for
"tty[S]X" prefix and it "eats" this prefix. The rest of *str buffer
is passed via *options parameter down to __add_preferred_console().
Best Regards,
Petr
(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)
{
bool found = false, first = true;
char *item, *opt = options;
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) = ',';
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;
}
Chris Down writes:
>Any thoughts on something simple like this that takes advantage of
>memmove()? This should overcome the mmio/io concerns, and it's fairly
>simple.
(although coming to think of it, this can just use memcpy(), but the same idea
applies)
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
On Mon 2023-04-17 16:08:11, Chris Down wrote:
> Chris Down writes:
> > Any thoughts on something simple like this that takes advantage of
> > memmove()? This should overcome the mmio/io concerns, and it's fairly
> > simple.
>
> (although coming to think of it, this can just use memcpy(), but the same
> idea applies)
I think that we have to use memmove() because the locations might be
overlaping. The to-be-moved options might be longer than the replaced
option.
Best Regards,
Petr
Thanks for the feedback! Ack to everything and will change for v4.