Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Jul 2023 03:49:45 -0700
From:   Breno Leitao <leitao@debian.org>
To:     Benjamin Poirier <benjamin.poirier@gmail.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, leit@meta.com,
        "open list:NETWORKING DRIVERS" <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] netconsole: Enable compile time configuration
Message-ID: <ZMOdSYry77qsRylG@gmail.com>
References: <20230727163132.745099-1-leitao@debian.org>
 <ZMK70fqdnfMPpc1x@d3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZMK70fqdnfMPpc1x@d3>
Precedence: bulk

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.