Received-SPF: pass (google.com: domain of linux-kernel+bounces-88191-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 1 Mar 2024 01:35:44 -0800
From: Breno Leitao <leitao@debian.org>
To: Matthew Wood <thepacketgeek@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>, netdev@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2] net: netconsole: Add continuation line
 prefix to userdata messages
Message-ID: <ZeGhcN/C7h1KWcTO@gmail.com>
References: <20240229183602.321747-1-thepacketgeek@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240229183602.321747-1-thepacketgeek@gmail.com>

On Thu, Feb 29, 2024 at 10:36:01AM -0800, Matthew Wood wrote:
> Add a space (' ') prefix to every userdata line to match docs for
> dev-kmsg. To account for this extra character in each userdata entry,
> reduce userdata entry names (directory name) from 54 characters to 53.
> 
> According to the dev-kmsg docs, a space is used for subsequent lines to
> mark them as continuation lines.
> 
> > A line starting with ' ', is a continuation line, adding
> > key/value pairs to the log message, which provide the machine
> > readable context of the message, for reliable processing in
> > userspace.
> 
> Testing for this patch::
> 
>  cd /sys/kernel/config/netconsole && mkdir cmdline0
>  cd cmdline0
>  mkdir userdata/test && echo "hello" > userdata/test/value
>  mkdir userdata/test2 && echo "hello2" > userdata/test2/value
>  echo "message" > /dev/kmsg
> 
> Outputs::
> 
>  6.8.0-rc5-virtme,12,493,231373579,-;message
>   test=hello
>   test2=hello2
> 
> And I confirmed all testing works as expected from the original patchset
> 
> v1 -> v2:
> - Calculate 53 byte user data name from: entry length - formatting chars - value length
> - Update docs to reflect 53 byte limit for user data name (director)

I think the changelog needs to come after the --- below, but I will
defer that to the maintainers.

> Fixes: df03f830d099 ("net: netconsole: cache userdata formatted string in netconsole_target")
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
>  Documentation/networking/netconsole.rst | 8 ++++----
>  drivers/net/netconsole.c                | 8 +++++---
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
> index b28c525e5d1e..d55c2a22ec7a 100644
> --- a/Documentation/networking/netconsole.rst
> +++ b/Documentation/networking/netconsole.rst
> @@ -180,7 +180,7 @@ Custom user data can be appended to the end of messages with netconsole
>  dynamic configuration enabled. User data entries can be modified without
>  changing the "enabled" attribute of a target.
>  
> -Directories (keys) under `userdata` are limited to 54 character length, and
> +Directories (keys) under `userdata` are limited to 53 character length, and
>  data in `userdata/<key>/value` are limited to 200 bytes::
>  
>   cd /sys/kernel/config/netconsole && mkdir cmdline0
> @@ -197,8 +197,8 @@ Messages will now include this additional user data::
>  Sends::
>  
>   12,607,22085407756,-;This is a message
> - foo=bar
> - qux=baz
> +  foo=bar
> +  qux=baz
>  
>  Preview the userdata that will be appended with::
>  
> @@ -218,7 +218,7 @@ The `qux` key is omitted since it has no value::
>  
>   echo "This is a message" > /dev/kmsg
>   12,607,22085407756,-;This is a message
> - foo=bar
> +  foo=bar
>  
>  Delete `userdata` entries with `rmdir`::
>  
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 0de108a1c0c8..46e447ea41b8 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -43,9 +43,11 @@ MODULE_DESCRIPTION("Console driver for network interfaces");
>  MODULE_LICENSE("GPL");
>  
>  #define MAX_PARAM_LENGTH	256
> -#define MAX_USERDATA_NAME_LENGTH	54
> -#define MAX_USERDATA_VALUE_LENGTH	200
>  #define MAX_USERDATA_ENTRY_LENGTH	256
> +#define MAX_USERDATA_VALUE_LENGTH	200
> +#define MAX_USERDATA_NAME_LENGTH	MAX_USERDATA_ENTRY_LENGTH - \
> +					MAX_USERDATA_VALUE_LENGTH - \
> +					3 /* ' ' '=' '\n' characters */

This is not beautiful even for my standards. I think something like the
code below makes more sense, even if checkpatch is not happy.

/* The number three below comes from ' ' + '=' + '\n' characters */
#define MAX_USERDATA_NAME_LENGTH 	MAX_USERDATA_ENTRY_LENGTH - MAX_USERDATA_VALUE_LENGTH - 3