Messages in this thread | | | Date | Thu, 10 Nov 2022 11:59:49 +0300 | From | Evgeniy Baskov <> | Subject | Re: [PATCH v7 2/5] x86: Add cmdline_prepare() helper |
| |
On 2022-11-10 01:54, Borislav Petkov wrote: > On Tue, Oct 04, 2022 at 03:48:21PM +0300, Evgeniy Baskov wrote: >> +static inline void cmdline_prepare(char *command_line, >> + const char *boot_command_line) >> +{ >> + if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) { >> + strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE); >> + } else if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) { >> + /* Append boot loader cmdline to builtin one. */ >> + strlcat(command_line, " ", COMMAND_LINE_SIZE); >> + strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE); >> + } > > Why can't we make this plain and simple and understandable at a quick > glance instead of putting parts of the string in some variable and then > still doing copying outside of the function? > > IOW, simply put everything in a single function: > > static inline void cmdline_prepare(char *dst, > const char *builtin_cmdline, > const char *boot_command_line) > { > /* Depends on CONFIG_CMDLINE_BOOL, overwrite with builtin > cmdline */ > if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) > strscpy(dst, builtin_cmdline, COMMAND_LINE_SIZE); > else if (IS_ENABLED(CONFIG_CMDLINE_BOOL)) { > if (builtin_cmdline[0]) { > /* append boot loader cmdline to builtin */ > srtlcat(dst, builtin_cmdline, > COMMAND_LINE_SIZE) > strlcat(dst, " ", COMMAND_LINE_SIZE); > strlcat(dst, boot_command_line, > COMMAND_LINE_SIZE); > } else { > strscpy(dst, boot_command_line, > COMMAND_LINE_SIZE); > } > } > } > > which is then called like this: > > cmdline_prepare(command_line, builtin_cmdline, boot_command_line); > > and when it returns command_line is ready. And before it, command_line > is the empty string: > > static char __initdata command_line[COMMAND_LINE_SIZE]; > > And all the complexity is in one single function and that single > function does each case in an obvious manner and you don't have to go > look outside of the function body to understand what it does. > > Hmmm?
Well, my version takes one less copy... But clarity is more important in the kernel, I guess. I'll send updated version soon.
Thanks, Evgeniy Baskov
| |