Re: [PATCH net-next v3] netconsole: Enable compile time configuration

[Eric Dumazet]

On Tue, Aug 1, 2023 at 12:06 PM Breno Leitao <leitao@xxxxxxxxxx> 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 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@xxxxxxxxxx>
> ---
>   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 configfs.
>           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 by
> +         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> for
> +         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 = true;
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_PREPEND_RELEASE))
> +               nt->release = true;
> +
>         nt->np.name = "netconsole";
>         strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
>         nt->np.local_port = 6665;
> @@ -681,6 +686,11 @@ static struct config_item *make_netconsole_target(struct config_group *group,
>         nt->np.remote_port = 6666;
>         eth_broadcast_addr(nt->np.remote_mac);
>
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_EXTENDED_LOG))
> +               nt->extended = true;
> +       if (IS_ENABLED(CONFIG_NETCONSOLE_PREPEND_RELEASE))
> +               nt->release = 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..a022ceaa2e3c4783ca0ea61c1c7b9f911d087abc
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 = kzalloc(sizeof(*nt), GFP_KERNEL);
+
+       if (nt) {
+               nt->np.name = "netconsole";
+               strscpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+               nt->np.local_port = 6665;
+               nt->np.remote_port = 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 = alloc_and_init();
        int err = -ENOMEM;
-       struct netconsole_target *nt;

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

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

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