Received-SPF: pass (google.com: domain of linux-kernel+bounces-94778-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240301185855.944405-1-thepacketgeek@gmail.com> <8a3ccdc5445d0cfda36418dd50746f13f447bdaa.camel@redhat.com>
In-Reply-To: <8a3ccdc5445d0cfda36418dd50746f13f447bdaa.camel@redhat.com>
From: Matthew Wood <thepacketgeek@gmail.com>
Date: Wed, 6 Mar 2024 16:25:58 -0800
Message-ID: <CADvopvYwZWsCtx6Vg42VHczexmX=0n5ZN99QAX3S=1aKzMAAWA@mail.gmail.com>
Subject: Re: [PATCH net-next v3] net: netconsole: Add continuation line prefix
 to userdata messages
To: Paolo Abeni <pabeni@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Jonathan Corbet <corbet@lwn.net>, Breno Leitao <leitao@debian.org>, 
	netdev@vger.kernel.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 4:14=E2=80=AFAM Paolo Abeni <pabeni@redhat.com> wrot=
e:
>
> On Fri, 2024-03-01 at 10:58 -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=3Dhello
> >   test2=3Dhello2
> >
> > And I confirmed all testing works as expected from the original patchse=
t
> >
> > Fixes: df03f830d099 ("net: netconsole: cache userdata formatted string =
in netconsole_target")
> > Reviewed-by: Breno Leitao <leitao@debian.org>
>
> I guess the tag arrived off-list, because I can't see any sign of it on
> the ML?!?
>
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > ---
> >
> > v1 -> v2:
> > - Calculate 53 byte user data name from: entry length - formatting char=
s - value length
> > - Update docs to reflect 53 byte limit for user data name (director)
> > v2 -> v3:
> > - Added #define for userdata formatting character length (3)
> > - Matched all #defines indent level
> >
> >  Documentation/networking/netconsole.rst |  8 ++++----
> >  drivers/net/netconsole.c                | 14 +++++++++-----
> >  2 files changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/networking/netconsole.rst b/Documentation/ne=
tworking/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 mess=
ages with netconsole
> >  dynamic configuration enabled. User data entries can be modified witho=
ut
> >  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=3Dbar
> > - qux=3Dbaz
> > +  foo=3Dbar
> > +  qux=3Dbaz
> >
> >  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=3Dbar
> > +  foo=3Dbar
> >
> >  Delete `userdata` entries with `rmdir`::
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 0de108a1c0c8..8cc28aec59c8 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -42,12 +42,16 @@ MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@seleni=
c.com>");
> >  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_PARAM_LENGTH             256
> > +/* characters used for formatting each userdata entry line (' ', '=3D'=
, '\n') */
> > +#define USERDATA_FORMAT_CHARS                3
> >  #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 - \
> > +                                     USERDATA_FORMAT_CHARS
>
> AFAICS this is not what Breno asked, and checkpatch complains
> rightfully. More importantly it's fragile: what will be the result of
>
>         MAX_USERDATA_NAME_LENGTH * 2
>
> ?
>
> At least some brackets are required:
>
> #define MAX_USERDATA_NAME_LENGTH        (MAX_USERDATA_ENTRY_LENGTH -
>                                          MAX_USERDATA_VALUE_LENGTH - \
>                                          USERDATA_FORMAT_CHARS)
>
> Thanks,
>
> Paolo
>

Thank you Paolo, great points. I will fix the macro and remove the
Reviewed-by tag so Breno can reply on the ML