Received-SPF: pass (google.com: domain of linux-kernel+bounces-133769-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sat, 6 Apr 2024 11:11:08 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: paulmck@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
 linux-fsdevel@vger.kernel.org, Zhenhua Huang <quic_zhenhuah@quicinc.com>
Subject: Re: [PATCH fs/proc/bootconfig] remove redundant comments from
 /proc/bootconfig
Message-Id: <20240406111108.e9a8b8c4cb8f44a8fb95b541@kernel.org>
In-Reply-To: <ef8cf3e7-9684-4495-a70e-c8f13ad188c5@paulmck-laptop>
References: <f036c5b0-20cc-40c1-85f9-69fa9edd0c95@paulmck-laptop>
	<20240404085522.63bf8cce6f961c07c8ce3f17@kernel.org>
	<26d56fa5-2c95-46da-8268-35642f857d6d@paulmck-laptop>
	<20240405102324.b7bb9fa052754d352cd2708e@kernel.org>
	<20240405115745.9b95679aa3ac516995d4d885@kernel.org>
	<ef8cf3e7-9684-4495-a70e-c8f13ad188c5@paulmck-laptop>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 4 Apr 2024 21:25:41 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote:
> > On Fri, 5 Apr 2024 10:23:24 +0900
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > 
> > > On Thu, 4 Apr 2024 10:43:14 -0700
> > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > 
> > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote:
> > > > > On Wed, 3 Apr 2024 12:16:28 -0700
> > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > 
> > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to
> > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig.
> > > > > > 
> > > > > > /proc/bootconfig shows boot_command_line[] multiple times following
> > > > > > every xbc key value pair, that's duplicated and not necessary.
> > > > > > Remove redundant ones.
> > > > > > 
> > > > > > Output before and after the fix is like:
> > > > > > key1 = value1
> > > > > > *bootloader argument comments*
> > > > > > key2 = value2
> > > > > > *bootloader argument comments*
> > > > > > key3 = value3
> > > > > > *bootloader argument comments*
> > > > > > ...
> > > > > > 
> > > > > > key1 = value1
> > > > > > key2 = value2
> > > > > > key3 = value3
> > > > > > *bootloader argument comments*
> > > > > > ...
> > > > > > 
> > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig")
> > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > > Cc: <linux-trace-kernel@vger.kernel.org>
> > > > > > Cc: <linux-fsdevel@vger.kernel.org>
> > > > > 
> > > > > OOps, good catch! Let me pick it.
> > > > > 
> > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > 
> > > > Thank you, and I have applied your ack and pulled this into its own
> > > > bootconfig.2024.04.04a.
> > > > 
> > > > My guess is that you will push this via your own tree, and so I will
> > > > drop my copy as soon as yours hits -next.
> > > 
> > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2.
> > 
> > Hmm I found that this always shows the command line comment in
> > /proc/bootconfig even without "bootconfig" option.
> > I think that is easier for user-tools but changes the behavior and
> > a bit redundant.
> > 
> > We should skip showing this original argument comment if bootconfig is
> > not initialized (no "bootconfig" in cmdline) as it is now.
> 
> So something like this folded into that patch?

Hm, I expected just checking it in the loop as below.

------------------------------------------------------------------------
diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
index e5635a6b127b..98e0780f7e07 100644
--- a/fs/proc/bootconfig.c
+++ b/fs/proc/bootconfig.c
@@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
 {
 	struct xbc_node *leaf, *vnode;
 	char *key, *end = dst + size;
+	bool empty = true;
 	const char *val;
 	char q;
 	int ret = 0;
@@ -62,8 +63,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
 				break;
 			dst += ret;
 		}
+		empty = false;
 	}
-	if (ret >= 0 && boot_command_line[0]) {
+	if (!empty && ret >= 0 && boot_command_line[0]) {
 		ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n",
 			       boot_command_line);
 		if (ret > 0)

------------------------------------------------------------------------

The difference is checking "bootconfig" cmdline option or checking
the "bootconfig" is actually empty. So the behaviors are different
when the "bootconfig" is specified but there is no bootconfig data.

Another idea is to check whether the cmdline is actually updated by
bootconfig and show original one only if it is updated.
(I think this fits the purpose of the original patch better.)

------------------------------------------------------------------------
diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
index e5635a6b127b..95d6a231210c 100644
--- a/fs/proc/bootconfig.c
+++ b/fs/proc/bootconfig.c
@@ -10,6 +10,9 @@
 #include <linux/bootconfig.h>
 #include <linux/slab.h>
 
+/* defined in main/init.c */
+bool __init cmdline_has_extra_options(void);
+
 static char *saved_boot_config;
 
 static int boot_config_proc_show(struct seq_file *m, void *v)
@@ -63,7 +66,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
 			dst += ret;
 		}
 	}
-	if (ret >= 0 && boot_command_line[0]) {
+	if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) {
 		ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n",
 			       boot_command_line);
 		if (ret > 0)
diff --git a/init/main.c b/init/main.c
index 2ca52474d0c3..881f6230ee59 100644
--- a/init/main.c
+++ b/init/main.c
@@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str)
 
 early_param("bootconfig", warn_bootconfig);
 
+bool __init cmdline_has_extra_options(void)
+{
+	return extra_command_line || extra_init_args;
+}
+
 /* Change NUL term back to "=", to make "param" the whole string. */
 static void __init repair_env_string(char *param, char *val)
 {
------------------------------------------------------------------------

Thank you,

> 
> ------------------------------------------------------------------------
> 
> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> index e5635a6b127b0..7d2520378f5f2 100644
> --- a/fs/proc/bootconfig.c
> +++ b/fs/proc/bootconfig.c
> @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
>  			dst += ret;
>  		}
>  	}
> -	if (ret >= 0 && boot_command_line[0]) {
> +	if (bootconfig_is_present() && ret >= 0 && boot_command_line[0]) {
>  		ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n",
>  			       boot_command_line);
>  		if (ret > 0)
> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> index ca73940e26df8..ef70d1b381421 100644
> --- a/include/linux/bootconfig.h
> +++ b/include/linux/bootconfig.h
> @@ -10,6 +10,7 @@
>  #ifdef __KERNEL__
>  #include <linux/kernel.h>
>  #include <linux/types.h>
> +int bootconfig_is_present(void);
>  #else /* !__KERNEL__ */
>  /*
>   * NOTE: This is only for tools/bootconfig, because tools/bootconfig will
> diff --git a/init/main.c b/init/main.c
> index 2ca52474d0c30..720a669b1493d 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1572,3 +1572,8 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	integrity_load_keys();
>  }
> +
> +int bootconfig_is_present(void)
> +{
> +	return bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE);
> +}
> 
> ------------------------------------------------------------------------
> 
> Give or take placement of the bootconfig_is_present() function's
> declaration and definition.
> 
> 							Thanx, Paul
> 
> 							Thanx, Paul
> 
> > Thank you,
> > 
> > 
> > > Thank you,
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > Thank you!
> > > > > 
> > > > > > 
> > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> > > > > > index 902b326e1e560..e5635a6b127b0 100644
> > > > > > --- a/fs/proc/bootconfig.c
> > > > > > +++ b/fs/proc/bootconfig.c
> > > > > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> > > > > >  				break;
> > > > > >  			dst += ret;
> > > > > >  		}
> > > > > > -		if (ret >= 0 && boot_command_line[0]) {
> > > > > > -			ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n",
> > > > > > -				       boot_command_line);
> > > > > > -			if (ret > 0)
> > > > > > -				dst += ret;
> > > > > > -		}
> > > > > > +	}
> > > > > > +	if (ret >= 0 && boot_command_line[0]) {
> > > > > > +		ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n",
> > > > > > +			       boot_command_line);
> > > > > > +		if (ret > 0)
> > > > > > +			dst += ret;
> > > > > >  	}
> > > > > >  out:
> > > > > >  	kfree(key);
> > > > > 
> > > > > 
> > > > > -- 
> > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > 
> > > -- 
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > 
> > -- 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>