Messages in this thread | | | Date | Tue, 18 Apr 2023 15:41:31 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options |
| |
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
| |