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;
MIME-Version: 1.0
References: <20230801100533.3350037-1-leitao@debian.org>
In-Reply-To: <20230801100533.3350037-1-leitao@debian.org>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 3 Aug 2023 09:47:43 +0200
Message-ID: <CANn89iKuHxUGphhDkKz2ZWS3YR3-BkieTb4b4gKMR9B7jxKpWQ@mail.gmail.com>
Subject: Re: [PATCH net-next v3] netconsole: Enable compile time configuration
To:     Breno Leitao <leitao@debian.org>
Cc:     rdunlap@infradead.org, benjamin.poirier@gmail.com,
        "David S. Miller" <davem@davemloft.net>,
        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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 1, 2023 at 12:06=E2=80=AFPM Breno Leitao <leitao@debian.org> wr=
ote:
>
> Enable netconsole features to be set at compilation time. Create two
> Kconfig options that allow users to set extended logs and release
> prepending features 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, simplifying the command line.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   v1 -> v2:
>         * Improvements in the Kconfig help section.
>   v2 -> v3:
>         * Honour the Kconfig settings when creating sysfs targets
>         * Add "by default" in a Kconfig help.
> ---
>  drivers/net/Kconfig      | 22 ++++++++++++++++++++++
>  drivers/net/netconsole.c | 10 ++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 368c6f5b327e..55fb9509bcae 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -332,6 +332,28 @@ config NETCONSOLE_DYNAMIC
>           at runtime through a userspace interface exported using configf=
s.
>           See <file:Documentation/networking/netconsole.rst> for details.
>
> +config NETCONSOLE_EXTENDED_LOG
> +       bool "Set kernel extended message by default"
> +       depends on NETCONSOLE
> +       default n
> +       help
> +         Set extended log support for netconsole message. If this option=
 is
> +         set, log messages are transmitted with extended metadata header=
 in a
> +         format similar to /dev/kmsg.  See
> +         <file:Documentation/networking/netconsole.rst> for details.
> +
> +config NETCONSOLE_PREPEND_RELEASE
> +       bool "Prepend kernel release version in the message by default"
> +       depends on NETCONSOLE_EXTENDED_LOG
> +       default n
> +       help
> +         Set kernel release to be prepended to each netconsole message b=
y
> +         default. If this option is set, the kernel release is prepended=
 into
> +         the first field of every netconsole message, so, the netconsole
> +         server/peer can easily identify what kernel release is logging =
each
> +         message.  See <file:Documentation/networking/netconsole.rst> fo=
r
> +         details.
> +
>  config NETPOLL
>         def_bool NETCONSOLE
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 87f18aedd3bd..e3b6155f4529 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 =3D true;
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_PREPEND_RELEASE))
> +               nt->release =3D true;
> +
>         nt->np.name =3D "netconsole";
>         strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
>         nt->np.local_port =3D 6665;
> @@ -681,6 +686,11 @@ static struct config_item *make_netconsole_target(st=
ruct config_group *group,
>         nt->np.remote_port =3D 6666;
>         eth_broadcast_addr(nt->np.remote_mac);
>
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_EXTENDED_LOG))
> +               nt->extended =3D true;
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_PREPEND_RELEASE))
> +               nt->release =3D true;
> +

Instead of duplicating these, what about adding a preliminary helper
in a separate patch ?

Something like this:

 drivers/net/netconsole.c |   45 ++++++++++++++++++++----------------------=
---
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 87f18aedd3bd7b3da9d2481dc9898e94dd75917b..a022ceaa2e3c4783ca0ea61c1c7=
b9f911d087abc
100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -167,26 +167,32 @@ static void netconsole_target_put(struct
netconsole_target *nt)

 #endif /* CONFIG_NETCONSOLE_DYNAMIC */

+/*
+ * Allocate and initialize with defaults.
+ * Note that these targets get their config_item fields zeroed-out.
+ */
+static struct netconsole_target *alloc_and_init(void)
+{
+       struct netconsole_target *nt =3D kzalloc(sizeof(*nt), GFP_KERNEL);
+
+       if (nt) {
+               nt->np.name =3D "netconsole";
+               strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+               nt->np.local_port =3D 6665;
+               nt->np.remote_port =3D 6666;
+               eth_broadcast_addr(nt->np.remote_mac);
+       }
+       return nt;
+}
+
 /* Allocate new target (from boot/module param) and setup netpoll for it *=
/
 static struct netconsole_target *alloc_param_target(char *target_config)
 {
+       struct netconsole_target *nt =3D alloc_and_init();
        int err =3D -ENOMEM;
-       struct netconsole_target *nt;

-       /*
-        * Allocate and initialize with defaults.
-        * Note that these targets get their config_item fields zeroed-out.
-        */
-       nt =3D kzalloc(sizeof(*nt), GFP_KERNEL);
        if (!nt)
                goto fail;
-
-       nt->np.name =3D "netconsole";
-       strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
-       nt->np.local_port =3D 6665;
-       nt->np.remote_port =3D 6666;
-       eth_broadcast_addr(nt->np.remote_mac);
-
        if (*target_config =3D=3D '+') {
                nt->extended =3D true;
                target_config++;
@@ -664,23 +670,12 @@ static const struct config_item_type
netconsole_target_type =3D {
 static struct config_item *make_netconsole_target(struct config_group *gro=
up,
                                                  const char *name)
 {
+       struct netconsole_target *nt =3D alloc_and_init();
        unsigned long flags;
-       struct netconsole_target *nt;

-       /*
-        * Allocate and initialize with defaults.
-        * Target is disabled at creation (!enabled).
-        */
-       nt =3D kzalloc(sizeof(*nt), GFP_KERNEL);
        if (!nt)
                return ERR_PTR(-ENOMEM);

-       nt->np.name =3D "netconsole";
-       strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
-       nt->np.local_port =3D 6665;
-       nt->np.remote_port =3D 6666;
-       eth_broadcast_addr(nt->np.remote_mac);
-
        /* Initialize the config_item member */
        config_item_init_type_name(&nt->item, name, &netconsole_target_type=
);