diff mbox series

[net-next] netconsole: Enable compile time configuration

Message ID 20230727163132.745099-1-leitao@debian.org
State New
Headers
Series [net-next] netconsole: Enable compile time configuration |

Commit Message

Breno Leitao July 27, 2023, 4:31 p.m. UTC 
  Enable netconsole features to be set at compilation time. Create two
Kconfig options that allow users to set extended logs and release
prepending features enabled at compilation time.

Right now, the user needs to pass command line parameters to netconsole,
such as "+"/"r" to enable extended logs and version prepending features.

With these two options, the user could set the default values for the
features at compile time, and don't need to pass it in the command line
to get them enabled.

Signed-off-by: Breno Leitao <leitao@debian.org>

Comments

Jakub Kicinski July 27, 2023, 4:46 p.m. UTC | #1 
On Thu, 27 Jul 2023 09:31:32 -0700 Breno Leitao wrote:
> +config NETCONSOLE_EXTENDED_LOG
> +	bool "Enable kernel extended message"
> +	depends on NETCONSOLE
> +	default n
> +	help
> +	  Enable extended log support for netconsole. Log messages are
> +	  transmitted with extended metadata header in the following format
> +	  which is the same as /dev/kmsg.
> +	  See <file:Documentation/networking/netconsole.rst> for details.
> +
> +config NETCONSOLE_APPEND_RELEASE
> +	bool "Enable kernel release version in the message"

We need to say the word "default" in a few places, to make it very
clear these are defaults.

I'll defer to others on whether this option makes sense, as I'm
probably too familiar with the problem you're solving to be impartial :S

Also the posting is pooped, the diff is listed twice.
Keep in mind the 24 grace period for netdev postings:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#tl-dr
Benjamin Poirier July 27, 2023, 6:47 p.m. UTC | #2 
On 2023-07-27 09:31 -0700, Breno Leitao wrote:
> Enable netconsole features to be set at compilation time. Create two
> Kconfig options that allow users to set extended logs and release
> prepending features enabled at compilation time.
> 
> Right now, the user needs to pass command line parameters to netconsole,
> such as "+"/"r" to enable extended logs and version prepending features.
> 
> With these two options, the user could set the default values for the
> features at compile time, and don't need to pass it in the command line
> to get them enabled.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 368c6f5b327e..4d0c3c532e72 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -332,6 +332,26 @@ config NETCONSOLE_DYNAMIC
>  	  at runtime through a userspace interface exported using configfs.
>  	  See <file:Documentation/networking/netconsole.rst> for details.
> 
> +config NETCONSOLE_EXTENDED_LOG
> +	bool "Enable kernel extended message"
> +	depends on NETCONSOLE
> +	default n
> +	help
> +	  Enable extended log support for netconsole. Log messages are
> +	  transmitted with extended metadata header in the following format
> +	  which is the same as /dev/kmsg.
> +	  See <file:Documentation/networking/netconsole.rst> for details.
> +
> +config NETCONSOLE_APPEND_RELEASE
                     ^ PREPEND

> +	bool "Enable kernel release version in the message"
> +	depends on NETCONSOLE_EXTENDED_LOG
> +	default n
> +	help
> +	  Enable kernel release to be prepended to each netcons message. The
> +	  kernel version is prepended to the first message, so, the peer knows what
                                         ^ each

> +	  kernel version is send the messages.

"kernel release" is one thing and "kernel version" is another:
root@vsid:~# uname --kernel-release
6.5.0-rc2+
root@vsid:~# uname --kernel-version
#37 SMP PREEMPT_DYNAMIC Thu Jul 27 14:20:44 EDT 2023

This option relates to the kernel release so please use the correct
name consistently in the help text.

> +	  See <file:Documentation/networking/netconsole.rst> for details.
> +
>  config NETPOLL
>  	def_bool NETCONSOLE
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 87f18aedd3bd..3a74f8c9cfdb 100644

[...]

Why is it needed to change the default for these parameters? Is there a
case where it's not possible to specify those values in the netconsole=
parameter?

If the default is set to on, there is no way to disable it via the
command line or module parameter, right?
Randy Dunlap July 27, 2023, 10:51 p.m. UTC | #3 
On 7/27/23 09:31, Breno Leitao wrote:
> +config NETCONSOLE_APPEND_RELEASE
> +	bool "Enable kernel release version in the message"
> +	depends on NETCONSOLE_EXTENDED_LOG
> +	default n
> +	help
> +	  Enable kernel release to be prepended to each netcons message. The
> +	  kernel version is prepended to the first message, so, the peer knows what
> +	  kernel version is send the messages.

	                 is sending
or
	                 sends

> +	  See <file:Documentation/networking/netconsole.rst> for details.
Breno Leitao July 28, 2023, 10:49 a.m. UTC | #4 
On Thu, Jul 27, 2023 at 02:47:45PM -0400, Benjamin Poirier wrote:
> On 2023-07-27 09:31 -0700, Breno Leitao wrote:
> > Enable netconsole features to be set at compilation time. Create two
> > Kconfig options that allow users to set extended logs and release
> > prepending features enabled at compilation time.
> > 
> > Right now, the user needs to pass command line parameters to netconsole,
> > such as "+"/"r" to enable extended logs and version prepending features.
> > 
> > With these two options, the user could set the default values for the
> > features at compile time, and don't need to pass it in the command line
> > to get them enabled.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index 368c6f5b327e..4d0c3c532e72 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -332,6 +332,26 @@ config NETCONSOLE_DYNAMIC
> >  	  at runtime through a userspace interface exported using configfs.
> >  	  See <file:Documentation/networking/netconsole.rst> for details.
> > 
> > +config NETCONSOLE_EXTENDED_LOG
> > +	bool "Enable kernel extended message"
> > +	depends on NETCONSOLE
> > +	default n
> > +	help
> > +	  Enable extended log support for netconsole. Log messages are
> > +	  transmitted with extended metadata header in the following format
> > +	  which is the same as /dev/kmsg.
> > +	  See <file:Documentation/networking/netconsole.rst> for details.
> > +
> > +config NETCONSOLE_APPEND_RELEASE
>                      ^ PREPEND
> 
> > +	bool "Enable kernel release version in the message"
> > +	depends on NETCONSOLE_EXTENDED_LOG
> > +	default n
> > +	help
> > +	  Enable kernel release to be prepended to each netcons message. The
> > +	  kernel version is prepended to the first message, so, the peer knows what
>                                          ^ each
> 
> > +	  kernel version is send the messages.
> 
> "kernel release" is one thing and "kernel version" is another:
> root@vsid:~# uname --kernel-release
> 6.5.0-rc2+
> root@vsid:~# uname --kernel-version
> #37 SMP PREEMPT_DYNAMIC Thu Jul 27 14:20:44 EDT 2023
> 
> This option relates to the kernel release so please use the correct
> name consistently in the help text.

Good point. I will fix it in v2.

> > +	  See <file:Documentation/networking/netconsole.rst> for details.
> > +
> >  config NETPOLL
> >  	def_bool NETCONSOLE
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 87f18aedd3bd..3a74f8c9cfdb 100644
> 
> [...]
> 
> Why is it needed to change the default for these parameters? Is there a
> case where it's not possible to specify those values in the netconsole=
> parameter?
> 
> If the default is set to on, there is no way to disable it via the
> command line or module parameter, right?

This patch shouldn't be chagning the default value at all. The default
value should be =n, as it is today. Where do you see the default being
changed?

This is what the patch does, setting "default n".

    +config NETCONSOLE_EXTENDED_LOG
    +       bool "Enable kernel extended message"
    +       depends on NETCONSOLE
    +       default n

and

    +config NETCONSOLE_APPEND_RELEASE
    +       bool "Enable kernel release version in the message"
    +       depends on NETCONSOLE_EXTENDED_LOG
    +       default n


Thanks for the review.
Breno Leitao July 28, 2023, 10:50 a.m. UTC | #5 
On Thu, Jul 27, 2023 at 03:51:41PM -0700, Randy Dunlap wrote:
> 
> 
> On 7/27/23 09:31, Breno Leitao wrote:
> > +config NETCONSOLE_APPEND_RELEASE
> > +	bool "Enable kernel release version in the message"
> > +	depends on NETCONSOLE_EXTENDED_LOG
> > +	default n
> > +	help
> > +	  Enable kernel release to be prepended to each netcons message. The
> > +	  kernel version is prepended to the first message, so, the peer knows what
> > +	  kernel version is send the messages.
> 
> 	                 is sending
> or
> 	                 sends

Thanks. I will fix on v2.
diff mbox series

Patch

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 368c6f5b327e..4d0c3c532e72 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -332,6 +332,26 @@  config NETCONSOLE_DYNAMIC
 	  at runtime through a userspace interface exported using configfs.
 	  See <file:Documentation/networking/netconsole.rst> for details.

+config NETCONSOLE_EXTENDED_LOG
+	bool "Enable kernel extended message"
+	depends on NETCONSOLE
+	default n
+	help
+	  Enable extended log support for netconsole. Log messages are
+	  transmitted with extended metadata header in the following format
+	  which is the same as /dev/kmsg.
+	  See <file:Documentation/networking/netconsole.rst> for details.
+
+config NETCONSOLE_APPEND_RELEASE
+	bool "Enable kernel release version in the message"
+	depends on NETCONSOLE_EXTENDED_LOG
+	default n
+	help
+	  Enable kernel release to be prepended to each netcons message. The
+	  kernel version is prepended to the first message, so, the peer knows what
+	  kernel version is send the messages.
+	  See <file:Documentation/networking/netconsole.rst> for details.
+
 config NETPOLL
 	def_bool NETCONSOLE

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 87f18aedd3bd..3a74f8c9cfdb 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -181,6 +181,11 @@  static struct netconsole_target *alloc_param_target(char *target_config)
 	if (!nt)
 		goto fail;

+	if (IS_ENABLED(CONFIG_NETCONSOLE_EXTENDED_LOG))
+		nt->extended = true;
+	if (IS_ENABLED(CONFIG_NETCONSOLE_APPEND_RELEASE))
+		nt->extended = true;
+
 	nt->np.name = "netconsole";
 	strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
 	nt->np.local_port = 6665;
---
 drivers/net/Kconfig      | 20 ++++++++++++++++++++
 drivers/net/netconsole.c |  5 +++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 368c6f5b327e..4d0c3c532e72 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -332,6 +332,26 @@  config NETCONSOLE_DYNAMIC
 	  at runtime through a userspace interface exported using configfs.
 	  See <file:Documentation/networking/netconsole.rst> for details.
 
+config NETCONSOLE_EXTENDED_LOG
+	bool "Enable kernel extended message"
+	depends on NETCONSOLE
+	default n
+	help
+	  Enable extended log support for netconsole. Log messages are
+	  transmitted with extended metadata header in the following format
+	  which is the same as /dev/kmsg.
+	  See <file:Documentation/networking/netconsole.rst> for details.
+
+config NETCONSOLE_APPEND_RELEASE
+	bool "Enable kernel release version in the message"
+	depends on NETCONSOLE_EXTENDED_LOG
+	default n
+	help
+	  Enable kernel release to be prepended to each netcons message. The
+	  kernel version is prepended to the first message, so, the peer knows what
+	  kernel version is send the messages.
+	  See <file:Documentation/networking/netconsole.rst> for details.
+
 config NETPOLL
 	def_bool NETCONSOLE
 
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 87f18aedd3bd..c84bcf97d4e4 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -181,6 +181,11 @@  static struct netconsole_target *alloc_param_target(char *target_config)
 	if (!nt)
 		goto fail;
 
+	if (IS_ENABLED(CONFIG_NETCONSOLE_EXTENDED_LOG))
+		nt->extended = true;
+	if (IS_ENABLED(CONFIG_NETCONSOLE_APPEND_RELEASE))
+		nt->release = true;
+
 	nt->np.name = "netconsole";
 	strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
 	nt->np.local_port = 6665;