diff mbox series

[net-next,v2,3/8] net: netconsole: move newline trimming to function

Message ID 20240126231348.281600-4-thepacketgeek@gmail.com
State New
Headers
Series netconsole: Add userdata append support |

Commit Message

Matthew Wood Jan. 26, 2024, 11:13 p.m. UTC 
  Move newline trimming logic from `dev_name_store()` to a new function
(trim_newline()) for shared use in netconsole.c

Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 drivers/net/netconsole.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

--
2.43.0

Comments

Breno Leitao Jan. 30, 2024, 9:16 a.m. UTC | #1 
On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> Move newline trimming logic from `dev_name_store()` to a new function
> (trim_newline()) for shared use in netconsole.c
> 
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
>  drivers/net/netconsole.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 085350beca87..b280d06bf152 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
>  			    struct netconsole_target, group);
>  }
> 
> +/* Get rid of possible trailing newline, returning the new length */
> +static void trim_newline(char *s, size_t maxlen)
> +{
> +	size_t len;
> +
> +	len = strnlen(s, maxlen);
> +	if (s[len - 1] == '\n')
> +		s[len - 1] = '\0';
> +}

I am thinking about this one. Should we replace the first `\n` in the
file by `\0` no matter where it is? This will probably make it easier to
implement the netconsd, where we know it will be impossible to have `\n`
in the userdata.

Maybe something as:

	static inline void trim_newline(char *str)
	{
		char *pos = strchr(str, '\n');

		if (pos)
			*pos = '\0';
	}


All in all, this is a good clean up, which make the code easier to read.
Thanks!
Matthew Wood Feb. 1, 2024, 4:45 a.m. UTC | #2 
On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
>
> On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > Move newline trimming logic from `dev_name_store()` to a new function
> > (trim_newline()) for shared use in netconsole.c
> >
> > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > ---
> >  drivers/net/netconsole.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 085350beca87..b280d06bf152 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> >                           struct netconsole_target, group);
> >  }
> >
> > +/* Get rid of possible trailing newline, returning the new length */
> > +static void trim_newline(char *s, size_t maxlen)
> > +{
> > +     size_t len;
> > +
> > +     len = strnlen(s, maxlen);
> > +     if (s[len - 1] == '\n')
> > +             s[len - 1] = '\0';
> > +}
>
> I am thinking about this one. Should we replace the first `\n` in the
> file by `\0` no matter where it is? This will probably make it easier to
> implement the netconsd, where we know it will be impossible to have `\n`
> in the userdata.
>
> Maybe something as:
>
>         static inline void trim_newline(char *str)
>         {
>                 char *pos = strchr(str, '\n');
>
>                 if (pos)
>                         *pos = '\0';
>         }
>
>
> All in all, this is a good clean up, which make the code easier to read.
> Thanks!

I like this idea, I agree that only accepting userdata values upto the
first newline clears up the expectations for log output and parsing on
the receiving side. I would prefer that to the case where multiple
values (delimited by newlines) are somehow attempted with a single
key, seems like just using additional key/value pairs would be
cleaner.
Matthew Wood Feb. 1, 2024, 5:31 a.m. UTC | #3 
On Wed, Jan 31, 2024 at 8:45 PM Matthew Wood <thepacketgeek@gmail.com> wrote:
>
> On Tue, Jan 30, 2024 at 1:16 AM Breno Leitao <leitao@debian.org> wrote:
> >
> > On Fri, Jan 26, 2024 at 03:13:38PM -0800, Matthew Wood wrote:
> > > Move newline trimming logic from `dev_name_store()` to a new function
> > > (trim_newline()) for shared use in netconsole.c
> > >
> > > Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> > > ---
> > >  drivers/net/netconsole.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index 085350beca87..b280d06bf152 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -230,6 +230,16 @@ static struct netconsole_target *to_target(struct config_item *item)
> > >                           struct netconsole_target, group);
> > >  }
> > >
> > > +/* Get rid of possible trailing newline, returning the new length */
> > > +static void trim_newline(char *s, size_t maxlen)
> > > +{
> > > +     size_t len;
> > > +
> > > +     len = strnlen(s, maxlen);
> > > +     if (s[len - 1] == '\n')
> > > +             s[len - 1] = '\0';
> > > +}
> >
> > I am thinking about this one. Should we replace the first `\n` in the
> > file by `\0` no matter where it is? This will probably make it easier to
> > implement the netconsd, where we know it will be impossible to have `\n`
> > in the userdata.
> >
> > Maybe something as:
> >
> >         static inline void trim_newline(char *str)
> >         {
> >                 char *pos = strchr(str, '\n');
> >
> >                 if (pos)
> >                         *pos = '\0';
> >         }
> >
> >
> > All in all, this is a good clean up, which make the code easier to read.
> > Thanks!
>
> I like this idea, I agree that only accepting userdata values upto the
> first newline clears up the expectations for log output and parsing on
> the receiving side. I would prefer that to the case where multiple
> values (delimited by newlines) are somehow attempted with a single
> key, seems like just using additional key/value pairs would be
> cleaner.

In practice truncating at the first newline makes no difference as
printk, echo, and other methods seem to buffer and write per-line. So
in this example, the stored value will be "val2" with or without the
suggested change:

$ printf "val1\nval2" > userdata/testing/value
# This results in two calls to userdatum_value_store, the first with
"val1\n" and the second with "val2". "val2" remains as the latest
write.
$ cat userdata/testing/value
val2

I will add a warning about this possibly unexpected behavior in the
docs for v3 for the patch
diff mbox series

Patch

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 085350beca87..b280d06bf152 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -230,6 +230,16 @@  static struct netconsole_target *to_target(struct config_item *item)
 			    struct netconsole_target, group);
 }

+/* Get rid of possible trailing newline, returning the new length */
+static void trim_newline(char *s, size_t maxlen)
+{
+	size_t len;
+
+	len = strnlen(s, maxlen);
+	if (s[len - 1] == '\n')
+		s[len - 1] = '\0';
+}
+
 /*
  * Attribute operations for netconsole_target.
  */
@@ -424,7 +434,6 @@  static ssize_t dev_name_store(struct config_item *item, const char *buf,
 		size_t count)
 {
 	struct netconsole_target *nt = to_target(item);
-	size_t len;

 	mutex_lock(&dynamic_netconsole_mutex);
 	if (nt->enabled) {
@@ -435,11 +444,7 @@  static ssize_t dev_name_store(struct config_item *item, const char *buf,
 	}

 	strscpy(nt->np.dev_name, buf, IFNAMSIZ);
-
-	/* Get rid of possible trailing newline from echo(1) */
-	len = strnlen(nt->np.dev_name, IFNAMSIZ);
-	if (nt->np.dev_name[len - 1] == '\n')
-		nt->np.dev_name[len - 1] = '\0';
+	trim_newline(nt->np.dev_name, IFNAMSIZ);

 	mutex_unlock(&dynamic_netconsole_mutex);
 	return strnlen(buf, count);