Re: [PATCH v7 2/5] x86: Add cmdline_prepare() helper

[Evgeniy Baskov]

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