[PATCH 1/5] CMDLINE: add generic builtin command line

[PATCH 1/5] CMDLINE: add generic builtin command line

[Daniel Walker]

This code allows architectures to use a generic builtin command line.
The state of the builtin command line options across architecture is
diverse. On x86 and mips they have pretty much the same code and the
code prepends the builtin command line onto the boot loader provided
one. On powerpc there is only a builtin override and nothing else.

The code in this commit unifies the code into a generic
header file under the CONFIG_GENERIC_CMDLINE option. When this
option is enabled the architecture can call the cmdline_add_builtin()
to add the builtin command line.

Cc: xe-linux-external@cisco.com
Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 include/linux/cmdline.h | 75 +++++++++++++++++++++++++++++++++++++++++
 init/Kconfig            | 68 +++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 include/linux/cmdline.h

diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..f44011d1a9ee
--- /dev/null
+++ b/include/linux/cmdline.h
@@ -0,0 +1,75 @@
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+/*
+ *
+ * Copyright (C) 2006,2021. Cisco Systems, Inc.
+ *
+ * Generic Append/Prepend cmdline support.
+ */
+
+#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
+
+#ifndef CONFIG_CMDLINE_OVERRIDE
+/*
+ * This function will append or prepend a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dest: The destination of the final appended/prepended string
+ * @src: The starting string or NULL if there isn't one.
+ * @tmp: temporary space used for prepending
+ * @length: the maximum length of the strings above.
+ */
+static inline void
+__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
+		size_t (*strlcpy)(char *dest, const char *src, size_t size),
+		size_t (*strlcat)(char *dest, const char *src, size_t count)
+		)
+{
+	if (src != dest && src != NULL) {
+		strlcpy(dest, " ", length);
+		strlcat(dest, src, length);
+	}
+
+	if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
+		strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
+
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
+		strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
+		strlcat(tmp, dest, length);
+		strlcpy(dest, tmp, length);
+	}
+}
+
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}
+#define cmdline_add_builtin(dest, src, length)	                           \
+	cmdline_add_builtin_custom(dest, src, length, __initdata, &strlcpy, &strlcat)
+#else
+#define cmdline_add_builtin(dest, src, length)				   \
+{								  	   \
+	strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND,    \
+		length);		   				   \
+}
+#endif /* !CONFIG_CMDLINE_OVERRIDE */
+
+#else
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
+	if (src != NULL) 							 \
+		strlcpy(dest, src, length);	 				 \
+}
+
+#define cmdline_add_builtin(dest, src, length) { 				\
+	cmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); 	\
+}
+#endif /* CONFIG_GENERIC_CMDLINE */
+
+
+#endif /* _LINUX_CMDLINE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 29ad68325028..28363ab07cd4 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2032,6 +2032,74 @@ config PROFILING
 config TRACEPOINTS
 	bool
 
+config GENERIC_CMDLINE
+	bool
+
+if GENERIC_CMDLINE
+
+config CMDLINE_BOOL
+	bool "Built-in kernel command line"
+	help
+	  Allow for specifying boot arguments to the kernel at
+	  build time.  On some systems (e.g. embedded ones), it is
+	  necessary or convenient to provide some or all of the
+	  kernel boot arguments with the kernel itself (that is,
+	  to not rely on the boot loader to provide them.)
+
+	  To compile command line arguments into the kernel,
+	  set this option to 'Y', then fill in the
+	  the boot arguments in CONFIG_CMDLINE.
+
+	  Systems with fully functional boot loaders (i.e. non-embedded)
+	  should leave this option set to 'N'.
+
+config CMDLINE_APPEND
+	string "Built-in kernel command string append"
+	depends on CMDLINE_BOOL
+	default ""
+	help
+	  Enter arguments here that should be compiled into the kernel
+	  image and used at boot time.  If the boot loader provides a
+	  command line at boot time, this string is appended to it to
+	  form the full kernel command line, when the system boots.
+
+	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
+	  change this behavior.
+
+	  In most cases, the command line (whether built-in or provided
+	  by the boot loader) should specify the device for the root
+	  file system.
+
+config CMDLINE_PREPEND
+	string "Built-in kernel command string prepend"
+	depends on CMDLINE_BOOL
+	default ""
+	help
+	  Enter arguments here that should be compiled into the kernel
+	  image and used at boot time.  If the boot loader provides a
+	  command line at boot time, this string is prepended to it to
+	  form the full kernel command line, when the system boots.
+
+	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
+	  change this behavior.
+
+	  In most cases, the command line (whether built-in or provided
+	  by the boot loader) should specify the device for the root
+	  file system.
+
+config CMDLINE_OVERRIDE
+	bool "Built-in command line overrides boot loader arguments"
+	depends on CMDLINE_BOOL
+	help
+	  Set this option to 'Y' to have the kernel ignore the boot loader
+	  command line, and use ONLY the built-in command line. In this case
+	  append and prepend strings are concatenated to form the full
+	  command line.
+
+	  This is used to work around broken boot loaders.  This should
+	  be set to 'N' under normal conditions.
+endif
+
 endmenu		# General setup
 
 source "arch/Kconfig"
-- 
2.25.1

Re: [PATCH 1/5] CMDLINE: add generic builtin command line

[Christophe Leroy]

Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> This code allows architectures to use a generic builtin command line.
> The state of the builtin command line options across architecture is
> diverse. On x86 and mips they have pretty much the same code and the
> code prepends the builtin command line onto the boot loader provided
> one. On powerpc there is only a builtin override and nothing else.

This is not exact. powerpc has:
CONFIG_FROM_BOOTLOADER
CONFIG_EXTEND
CONFIG_FORCE

> 
> The code in this commit unifies the code into a generic
> header file under the CONFIG_GENERIC_CMDLINE option. When this
> option is enabled the architecture can call the cmdline_add_builtin()
> to add the builtin command line.
> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   include/linux/cmdline.h | 75 +++++++++++++++++++++++++++++++++++++++++
>   init/Kconfig            | 68 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+)
>   create mode 100644 include/linux/cmdline.h
> 
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..f44011d1a9ee
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,75 @@

Missing the SPDX Licence Identifier

> +#ifndef _LINUX_CMDLINE_H
> +#define _LINUX_CMDLINE_H
> +
> +/*
> + *
> + * Copyright (C) 2006,2021. Cisco Systems, Inc.
> + *
> + * Generic Append/Prepend cmdline support.
> + */
> +
> +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)

I think it would be better if we can avoid the CONFIG_CMDLINE_BOOL.
By making the CMDLINEs default to "" at all time, I think we can about that BOOL.

> +
> +#ifndef CONFIG_CMDLINE_OVERRIDE
> +/*
> + * This function will append or prepend a builtin command line to the command

As far as I understand, it doesn't "append _or_ prepend" but it does "append _and_ prepend"

> + * line provided by the bootloader. Kconfig options can be used to alter
> + * the behavior of this builtin command line.
> + * @dest: The destination of the final appended/prepended string
> + * @src: The starting string or NULL if there isn't one.
> + * @tmp: temporary space used for prepending
> + * @length: the maximum length of the strings above.

Missing some parameters here, but I think we should avoid those 'strlcpy' and 'strlcat', see later 
comment.

> + */
> +static inline void
> +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
> +		size_t (*strlcpy)(char *dest, const char *src, size_t size),
> +		size_t (*strlcat)(char *dest, const char *src, size_t count)

Don't use names that overide names of existing functions.

'count' is __kernel_size_t not size_t

> +		)
> +{
> +	if (src != dest && src != NULL) {
> +		strlcpy(dest, " ", length);

Why do you need a space up front in that case ? Why not just copy the source to the destination ?

> +		strlcat(dest, src, length);
> +	}
> +
> +	if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> +		strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
> +
> +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> +		strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
> +		strlcat(tmp, dest, length);
> +		strlcpy(dest, tmp, length);

Could we use memmove(), or implement strmove() and avoid the temporary buffer at all ?

> +	}
> +}
> +
> +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\

It is misleading to call parameters 'strlcpy' or 'strlcat', it hides that they are overriden.

> +{ 												\
> +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
> +		static label char cmdline_tmp_space[length]; 					\

Let the architecture define the temporary space when using the custom variant instead of just asking 
the architecture to provide the name of the section to use. powerpc already have prom_scratch for that.

> +		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
> +	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
> +		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
> +	} 											\

Ah, so if I understand correctly, the user can set both CONFIG_CMDLINE_PREPEND and 
CONFIG_CMDLINE_APPEND but one of them is silently ignored.

Then I think we should just offer the user to set one, name it CONFIG_CMDLINE then ask him to choose 
between FORCE, APPEND or PREPEND.

> +}
> +#define cmdline_add_builtin(dest, src, length)	                           \
> +	cmdline_add_builtin_custom(dest, src, length, __initdata, &strlcpy, &strlcat)
> +#else
> +#define cmdline_add_builtin(dest, src, length)				   \
> +{								  	   \
> +	strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND,    \
> +		length);		   				   \
> +}
> +#endif /* !CONFIG_CMDLINE_OVERRIDE */
> +
> +#else
> +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
> +	if (src != NULL) 							 \
> +		strlcpy(dest, src, length);	 				 \
> +}
> +
> +#define cmdline_add_builtin(dest, src, length) { 				\
> +	cmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); 	\
> +}
> +#endif /* CONFIG_GENERIC_CMDLINE */

I'd rather avoid all those macros and use static inline functions instead.

For the strlcpy() and strlcat(), use another name, for instance cmdline_strlcpy and cmdline_strlcat. 
Then at the begining of the file, define them as strlcpy ad strlcat unless they are already defined 
to something else (by the architecture before including cmdline.h).

> +
> +
> +#endif /* _LINUX_CMDLINE_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 29ad68325028..28363ab07cd4 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2032,6 +2032,74 @@ config PROFILING
>   config TRACEPOINTS
>   	bool
>   
> +config GENERIC_CMDLINE
> +	bool
> +
> +if GENERIC_CMDLINE
> +
> +config CMDLINE_BOOL
> +	bool "Built-in kernel command line"

We don't need the CMDLINE_BOOL, just have CMDLINE always "" by default.

> +	help
> +	  Allow for specifying boot arguments to the kernel at
> +	  build time.  On some systems (e.g. embedded ones), it is
> +	  necessary or convenient to provide some or all of the
> +	  kernel boot arguments with the kernel itself (that is,
> +	  to not rely on the boot loader to provide them.)
> +
> +	  To compile command line arguments into the kernel,
> +	  set this option to 'Y', then fill in the
> +	  the boot arguments in CONFIG_CMDLINE.
> +
> +	  Systems with fully functional boot loaders (i.e. non-embedded)
> +	  should leave this option set to 'N'.
> +
> +config CMDLINE_APPEND

As far as I understand, the generic code will only take CMDLINE_APPEND into account if 
CMDLINE_PREPEND doesn't exist, otherwise it will silently ignore it.

Only offer one string: CONFIG_CMDLINE, and make the use choose between APPEND, EXTEND or OVERRIDE

> +	string "Built-in kernel command string append"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is appended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_PREPEND
> +	string "Built-in kernel command string prepend"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is prepended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_OVERRIDE
> +	bool "Built-in command line overrides boot loader arguments"
> +	depends on CMDLINE_BOOL
> +	help
> +	  Set this option to 'Y' to have the kernel ignore the boot loader
> +	  command line, and use ONLY the built-in command line. In this case
> +	  append and prepend strings are concatenated to form the full
> +	  command line.
> +
> +	  This is used to work around broken boot loaders.  This should
> +	  be set to 'N' under normal conditions.
> +endif
> +
>   endmenu		# General setup
>   
>   source "arch/Kconfig"
> 

Christophe

Re: [PATCH 1/5] CMDLINE: add generic builtin command line

[Christophe Leroy]

Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> This code allows architectures to use a generic builtin command line.
> The state of the builtin command line options across architecture is
> diverse. On x86 and mips they have pretty much the same code and the
> code prepends the builtin command line onto the boot loader provided
> one. On powerpc there is only a builtin override and nothing else.
> 
> The code in this commit unifies the code into a generic
> header file under the CONFIG_GENERIC_CMDLINE option. When this
> option is enabled the architecture can call the cmdline_add_builtin()
> to add the builtin command line.

WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#32: FILE: include/linux/cmdline.h:1:
+#ifndef _LINUX_CMDLINE_H

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#56: FILE: include/linux/cmdline.h:25:
+__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
+		size_t (*strlcpy)(char *dest, const char *src, size_t size),

WARNING:STRLCPY: Prefer strscpy over strlcpy - see: 
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
#61: FILE: include/linux/cmdline.h:30:
+		strlcpy(dest, " ", length);

WARNING:STRLCPY: Prefer strscpy over strlcpy - see: 
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
#69: FILE: include/linux/cmdline.h:38:
+		strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);

WARNING:STRLCPY: Prefer strscpy over strlcpy - see: 
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
#71: FILE: include/linux/cmdline.h:40:
+		strlcpy(dest, tmp, length);

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) ^I^I^I\$

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'dest' - possible side-effects?
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'src' - possible side-effects?
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'length' - possible side-effects?
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'strlcpy' - possible side-effects?
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'strlcat' - possible side-effects?
#75: FILE: include/linux/cmdline.h:44:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
+{ 												\
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
+		static label char cmdline_tmp_space[length]; 					\
+		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
+	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
+		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
+	} 											\
+}

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#76: FILE: include/linux/cmdline.h:45:
+{ ^I^I^I^I^I^I^I^I^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#77: FILE: include/linux/cmdline.h:46:
+^Iif (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { ^I^I^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#78: FILE: include/linux/cmdline.h:47:
+^I^Istatic label char cmdline_tmp_space[length]; ^I^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#79: FILE: include/linux/cmdline.h:48:
+^I^I__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); ^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#80: FILE: include/linux/cmdline.h:49:
+^I} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { ^I^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#81: FILE: include/linux/cmdline.h:50:
+^I^I__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); ^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#82: FILE: include/linux/cmdline.h:51:
+^I} ^I^I^I^I^I^I^I^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#88: FILE: include/linux/cmdline.h:57:
+{^I^I^I^I^I^I^I^I  ^I   \$

WARNING:STRLCPY: Prefer strscpy over strlcpy - see: 
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
#89: FILE: include/linux/cmdline.h:58:
+	strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND,    \

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#90: FILE: include/linux/cmdline.h:59:
+^I^Ilength);^I^I   ^I^I^I^I   \$

CHECK:MACRO_ARG_REUSE: Macro argument reuse 'src' - possible side-effects?
#95: FILE: include/linux/cmdline.h:64:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
+	if (src != NULL) 							 \
+		strlcpy(dest, src, length);	 				 \
+}

CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'src' may be better as '(src)' to avoid precedence issues
#95: FILE: include/linux/cmdline.h:64:
+#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
+	if (src != NULL) 							 \
+		strlcpy(dest, src, length);	 				 \
+}

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#96: FILE: include/linux/cmdline.h:65:
+^Iif (src != NULL) ^I^I^I^I^I^I^I \$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#97: FILE: include/linux/cmdline.h:66:
+^I^Istrlcpy(dest, src, length);^I ^I^I^I^I \$

WARNING:STRLCPY: Prefer strscpy over strlcpy - see: 
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
#97: FILE: include/linux/cmdline.h:66:
+		strlcpy(dest, src, length);	 				 \

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#100: FILE: include/linux/cmdline.h:69:
+#define cmdline_add_builtin(dest, src, length) { ^I^I^I^I\$

WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#101: FILE: include/linux/cmdline.h:70:
+^Icmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); ^I\$

total: 0 errors, 20 warnings, 8 checks, 149 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or --fix-inplace.

the.patch has style problems, please review.

NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH 
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.


> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   include/linux/cmdline.h | 75 +++++++++++++++++++++++++++++++++++++++++
>   init/Kconfig            | 68 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+)
>   create mode 100644 include/linux/cmdline.h
> 
> diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> new file mode 100644
> index 000000000000..f44011d1a9ee
> --- /dev/null
> +++ b/include/linux/cmdline.h
> @@ -0,0 +1,75 @@
> +#ifndef _LINUX_CMDLINE_H
> +#define _LINUX_CMDLINE_H
> +
> +/*
> + *
> + * Copyright (C) 2006,2021. Cisco Systems, Inc.
> + *
> + * Generic Append/Prepend cmdline support.
> + */
> +
> +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
> +
> +#ifndef CONFIG_CMDLINE_OVERRIDE
> +/*
> + * This function will append or prepend a builtin command line to the command
> + * line provided by the bootloader. Kconfig options can be used to alter
> + * the behavior of this builtin command line.
> + * @dest: The destination of the final appended/prepended string
> + * @src: The starting string or NULL if there isn't one.
> + * @tmp: temporary space used for prepending
> + * @length: the maximum length of the strings above.
> + */
> +static inline void
> +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
> +		size_t (*strlcpy)(char *dest, const char *src, size_t size),
> +		size_t (*strlcat)(char *dest, const char *src, size_t count)
> +		)
> +{
> +	if (src != dest && src != NULL) {
> +		strlcpy(dest, " ", length);
> +		strlcat(dest, src, length);
> +	}
> +
> +	if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> +		strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
> +
> +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> +		strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
> +		strlcat(tmp, dest, length);
> +		strlcpy(dest, tmp, length);
> +	}
> +}
> +
> +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
> +{ 												\
> +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
> +		static label char cmdline_tmp_space[length]; 					\
> +		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
> +	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
> +		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
> +	} 											\
> +}
> +#define cmdline_add_builtin(dest, src, length)	                           \
> +	cmdline_add_builtin_custom(dest, src, length, __initdata, &strlcpy, &strlcat)
> +#else
> +#define cmdline_add_builtin(dest, src, length)				   \
> +{								  	   \
> +	strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND,    \
> +		length);		   				   \
> +}
> +#endif /* !CONFIG_CMDLINE_OVERRIDE */
> +
> +#else
> +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
> +	if (src != NULL) 							 \
> +		strlcpy(dest, src, length);	 				 \
> +}
> +
> +#define cmdline_add_builtin(dest, src, length) { 				\
> +	cmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); 	\
> +}
> +#endif /* CONFIG_GENERIC_CMDLINE */
> +
> +
> +#endif /* _LINUX_CMDLINE_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 29ad68325028..28363ab07cd4 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2032,6 +2032,74 @@ config PROFILING
>   config TRACEPOINTS
>   	bool
>   
> +config GENERIC_CMDLINE
> +	bool
> +
> +if GENERIC_CMDLINE
> +
> +config CMDLINE_BOOL
> +	bool "Built-in kernel command line"
> +	help
> +	  Allow for specifying boot arguments to the kernel at
> +	  build time.  On some systems (e.g. embedded ones), it is
> +	  necessary or convenient to provide some or all of the
> +	  kernel boot arguments with the kernel itself (that is,
> +	  to not rely on the boot loader to provide them.)
> +
> +	  To compile command line arguments into the kernel,
> +	  set this option to 'Y', then fill in the
> +	  the boot arguments in CONFIG_CMDLINE.
> +
> +	  Systems with fully functional boot loaders (i.e. non-embedded)
> +	  should leave this option set to 'N'.
> +
> +config CMDLINE_APPEND
> +	string "Built-in kernel command string append"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is appended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_PREPEND
> +	string "Built-in kernel command string prepend"
> +	depends on CMDLINE_BOOL
> +	default ""
> +	help
> +	  Enter arguments here that should be compiled into the kernel
> +	  image and used at boot time.  If the boot loader provides a
> +	  command line at boot time, this string is prepended to it to
> +	  form the full kernel command line, when the system boots.
> +
> +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> +	  change this behavior.
> +
> +	  In most cases, the command line (whether built-in or provided
> +	  by the boot loader) should specify the device for the root
> +	  file system.
> +
> +config CMDLINE_OVERRIDE
> +	bool "Built-in command line overrides boot loader arguments"
> +	depends on CMDLINE_BOOL
> +	help
> +	  Set this option to 'Y' to have the kernel ignore the boot loader
> +	  command line, and use ONLY the built-in command line. In this case
> +	  append and prepend strings are concatenated to form the full
> +	  command line.
> +
> +	  This is used to work around broken boot loaders.  This should
> +	  be set to 'N' under normal conditions.
> +endif
> +
>   endmenu		# General setup
>   
>   source "arch/Kconfig"
> 

Re: [PATCH 1/5] CMDLINE: add generic builtin command line

[Daniel Walker]

On Thu, Mar 04, 2021 at 08:00:49AM +0100, Christophe Leroy wrote:
> 
> 
> Le 04/03/2021 à 05:47, Daniel Walker a écrit :
> > This code allows architectures to use a generic builtin command line.
> > The state of the builtin command line options across architecture is
> > diverse. On x86 and mips they have pretty much the same code and the
> > code prepends the builtin command line onto the boot loader provided
> > one. On powerpc there is only a builtin override and nothing else.
> 
> This is not exact. powerpc has:
> CONFIG_FROM_BOOTLOADER
> CONFIG_EXTEND
> CONFIG_FORCE
 
I don't currently have ppc64 to test on, but CONFIG_FROM_BOOTLOADER should likely
stay, but the other two can come from the generic code.


> > 
> > The code in this commit unifies the code into a generic
> > header file under the CONFIG_GENERIC_CMDLINE option. When this
> > option is enabled the architecture can call the cmdline_add_builtin()
> > to add the builtin command line.
> > 
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> >   include/linux/cmdline.h | 75 +++++++++++++++++++++++++++++++++++++++++
> >   init/Kconfig            | 68 +++++++++++++++++++++++++++++++++++++
> >   2 files changed, 143 insertions(+)
> >   create mode 100644 include/linux/cmdline.h
> > 
> > diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
> > new file mode 100644
> > index 000000000000..f44011d1a9ee
> > --- /dev/null
> > +++ b/include/linux/cmdline.h
> > @@ -0,0 +1,75 @@
> 
> Missing the SPDX Licence Identifier
> 
> > +#ifndef _LINUX_CMDLINE_H
> > +#define _LINUX_CMDLINE_H
> > +
> > +/*
> > + *
> > + * Copyright (C) 2006,2021. Cisco Systems, Inc.
> > + *
> > + * Generic Append/Prepend cmdline support.
> > + */
> > +
> > +#if defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_CMDLINE_BOOL)
> 
> I think it would be better if we can avoid the CONFIG_CMDLINE_BOOL.
> By making the CMDLINEs default to "" at all time, I think we can about that BOOL.

Wouldn't it be annoying if you have to deleted all the characters from two text
boxes vs. just disabling a single option ? What if you leave a space
accidentally , woops.

> > +
> > +#ifndef CONFIG_CMDLINE_OVERRIDE
> > +/*
> > + * This function will append or prepend a builtin command line to the command
> 
> As far as I understand, it doesn't "append _or_ prepend" but it does "append _and_ prepend"

I think the end results is accurately , no need to get pedantic.

> > + * line provided by the bootloader. Kconfig options can be used to alter
> > + * the behavior of this builtin command line.
> > + * @dest: The destination of the final appended/prepended string
> > + * @src: The starting string or NULL if there isn't one.
> > + * @tmp: temporary space used for prepending
> > + * @length: the maximum length of the strings above.
> 
> Missing some parameters here, but I think we should avoid those 'strlcpy'
> and 'strlcat', see later comment.
> 
> > + */
> > +static inline void
> > +__cmdline_add_builtin(char *dest, const char *src, char *tmp, unsigned long length,
> > +		size_t (*strlcpy)(char *dest, const char *src, size_t size),
> > +		size_t (*strlcat)(char *dest, const char *src, size_t count)
> 
> Don't use names that overide names of existing functions.
> 
> 'count' is __kernel_size_t not size_t
 
It's type checking all the parameters at compile time, it doesn't complain about
this that I've seen.


> > +		)
> > +{
> > +	if (src != dest && src != NULL) {
> > +		strlcpy(dest, " ", length);
> 
> Why do you need a space up front in that case ? Why not just copy the source to the destination ?

There may not be a space between them, it doesn't cost anything to have one.

> > +		strlcat(dest, src, length);
> > +	}
> > +
> > +	if (sizeof(CONFIG_CMDLINE_APPEND) > 1)
> > +		strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
> > +
> > +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) {
> > +		strlcpy(tmp, CONFIG_CMDLINE_PREPEND " ", length);
> > +		strlcat(tmp, dest, length);
> > +		strlcpy(dest, tmp, length);
> 
> Could we use memmove(), or implement strmove() and avoid the temporary buffer at all ?

I don't really want to make drastic alteration like this, unless there is a
better reason for it. Most of this hasn't change inside Cisco's tree for almost a decade.

> > +	}
> > +}
> > +
> > +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) 			\
> 
> It is misleading to call parameters 'strlcpy' or 'strlcat', it hides that they are overriden.

I can change the names, it's not a big deal.

> > +{ 												\
> > +	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1) { 						\
> > +		static label char cmdline_tmp_space[length]; 					\
> 
> Let the architecture define the temporary space when using the custom
> variant instead of just asking the architecture to provide the name of the
> section to use. powerpc already have prom_scratch for that.
 
How would it use this space exactly ? Is it large enough ? How is it managed?


> > +		__cmdline_add_builtin(dest, src, cmdline_tmp_space, length, strlcpy, strlcat); 	\
> > +	} else if (sizeof(CONFIG_CMDLINE_APPEND) > 1) { 					\
> > +		__cmdline_add_builtin(dest, src, NULL, length, strlcpy, strlcat); 		\
> > +	} 											\
> 
> Ah, so if I understand correctly, the user can set both
> CONFIG_CMDLINE_PREPEND and CONFIG_CMDLINE_APPEND but one of them is silently
> ignored.
 
Nothing should be ignored. Either one set gets you into the function, just one
has to create a variable.

> Then I think we should just offer the user to set one, name it
> CONFIG_CMDLINE then ask him to choose between FORCE, APPEND or PREPEND.

No, this doesn't work for Cisco. We need to functionality of this solution,
nothing less..

> > +}
> > +#define cmdline_add_builtin(dest, src, length)	                           \
> > +	cmdline_add_builtin_custom(dest, src, length, __initdata, &strlcpy, &strlcat)
> > +#else
> > +#define cmdline_add_builtin(dest, src, length)				   \
> > +{								  	   \
> > +	strlcpy(dest, CONFIG_CMDLINE_PREPEND " " CONFIG_CMDLINE_APPEND,    \
> > +		length);		   				   \
> > +}
> > +#endif /* !CONFIG_CMDLINE_OVERRIDE */
> > +
> > +#else
> > +#define cmdline_add_builtin_custom(dest, src, length, label, strlcpy, strlcat) { \
> > +	if (src != NULL) 							 \
> > +		strlcpy(dest, src, length);	 				 \
> > +}
> > +
> > +#define cmdline_add_builtin(dest, src, length) { 				\
> > +	cmdline_add_builtin_custom(dest, src, length, strlcpy, strlcat); 	\
> > +}
> > +#endif /* CONFIG_GENERIC_CMDLINE */
> 
> I'd rather avoid all those macros and use static inline functions instead.
 
The last two in the off case might be able to be converted.

> For the strlcpy() and strlcat(), use another name, for instance
> cmdline_strlcpy and cmdline_strlcat. Then at the begining of the file,
> define them as strlcpy ad strlcat unless they are already defined to
> something else (by the architecture before including cmdline.h).
 

Your duplicating your comments.


> > +
> > +
> > +#endif /* _LINUX_CMDLINE_H */
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 29ad68325028..28363ab07cd4 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2032,6 +2032,74 @@ config PROFILING
> >   config TRACEPOINTS
> >   	bool
> > +config GENERIC_CMDLINE
> > +	bool
> > +
> > +if GENERIC_CMDLINE
> > +
> > +config CMDLINE_BOOL
> > +	bool "Built-in kernel command line"
> 
> We don't need the CMDLINE_BOOL, just have CMDLINE always "" by default.
 
I think it's more usable as explained above.


> > +	help
> > +	  Allow for specifying boot arguments to the kernel at
> > +	  build time.  On some systems (e.g. embedded ones), it is
> > +	  necessary or convenient to provide some or all of the
> > +	  kernel boot arguments with the kernel itself (that is,
> > +	  to not rely on the boot loader to provide them.)
> > +
> > +	  To compile command line arguments into the kernel,
> > +	  set this option to 'Y', then fill in the
> > +	  the boot arguments in CONFIG_CMDLINE.
> > +
> > +	  Systems with fully functional boot loaders (i.e. non-embedded)
> > +	  should leave this option set to 'N'.
> > +
> > +config CMDLINE_APPEND
> 
> As far as I understand, the generic code will only take CMDLINE_APPEND into
> account if CMDLINE_PREPEND doesn't exist, otherwise it will silently ignore
> it.

No, that's not how that works.

> Only offer one string: CONFIG_CMDLINE, and make the use choose between APPEND, EXTEND or OVERRIDE

No. That's not how this works.

> > +	string "Built-in kernel command string append"
> > +	depends on CMDLINE_BOOL
> > +	default ""
> > +	help
> > +	  Enter arguments here that should be compiled into the kernel
> > +	  image and used at boot time.  If the boot loader provides a
> > +	  command line at boot time, this string is appended to it to
> > +	  form the full kernel command line, when the system boots.
> > +
> > +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> > +	  change this behavior.
> > +
> > +	  In most cases, the command line (whether built-in or provided
> > +	  by the boot loader) should specify the device for the root
> > +	  file system.
> > +
> > +config CMDLINE_PREPEND
> > +	string "Built-in kernel command string prepend"
> > +	depends on CMDLINE_BOOL
> > +	default ""
> > +	help
> > +	  Enter arguments here that should be compiled into the kernel
> > +	  image and used at boot time.  If the boot loader provides a
> > +	  command line at boot time, this string is prepended to it to
> > +	  form the full kernel command line, when the system boots.
> > +
> > +	  However, you can use the CONFIG_CMDLINE_OVERRIDE option to
> > +	  change this behavior.
> > +
> > +	  In most cases, the command line (whether built-in or provided
> > +	  by the boot loader) should specify the device for the root
> > +	  file system.
> > +
> > +config CMDLINE_OVERRIDE
> > +	bool "Built-in command line overrides boot loader arguments"
> > +	depends on CMDLINE_BOOL
> > +	help
> > +	  Set this option to 'Y' to have the kernel ignore the boot loader
> > +	  command line, and use ONLY the built-in command line. In this case
> > +	  append and prepend strings are concatenated to form the full
> > +	  command line.
> > +
> > +	  This is used to work around broken boot loaders.  This should
> > +	  be set to 'N' under normal conditions.
> > +endif
> > +
> >   endmenu		# General setup
> >   source "arch/Kconfig"
> > 
> 
> Christophe


Most of your comments are the kind of things this code went thru on it's first
implementation, and were discarded for a reason during usage and testing.

Daniel