[PATCH net-next 0/9] netconsole refactoring and warning fix

[PATCH net-next 0/9] netconsole refactoring and warning fix

[Breno Leitao]

The netconsole driver was showing a warning related to userdata
information, depending on the message size being transmitted:

	------------[ cut here ]------------
	WARNING: CPU: 13 PID: 3013042 at drivers/net/netconsole.c:1122 write_ext_msg+0x3b6/0x3d0
	 ? write_ext_msg+0x3b6/0x3d0
	 console_flush_all+0x1e9/0x330
	 ...

Identifying the cause of this warning proved to be non-trivial due to:

 * The write_ext_msg() function being over 100 lines long
 * Extensive use of pointer arithmetic
 * Inconsistent naming conventions and concept application

The send_ext_msg() function grew organically over time:

 * Initially, the UDP packet consisted of a header and body
 * Later additions included release prepend and userdata
 * Naming became inconsistent (e.g., "body" excludes userdata, "header"
   excludes prepended release)

This lack of consistency made investigating issues like the above warning
more challenging than what it should be.

To address these issues, the following steps were taken:

 * Breaking down write_ext_msg() into smaller functions with clear scopes
 * Improving readability and reasoning about the code
 * Simplifying and clarifying naming conventions

Warning Fix
-----------

The warning occurred when there was insufficient buffer space to append
userdata. While this scenario is acceptable (as userdata can be sent in a
separate packet later), the kernel was incorrectly raising a warning.  A
one-line fix has been implemented to resolve this issue.

A self-test was developed to write messages of every possible length
This test will be submitted in a separate patchset

Breno Leitao (9):
  net: netconsole: remove msg_ready variable
  net: netconsole: split send_ext_msg_udp() function
  net: netconsole: separate fragmented message handling in send_ext_msg
  net: netconsole: rename body to msg_body
  net: netconsole: introduce variable to track body length
  net: netconsole: track explicitly if msgbody was written to buffer
  net: netconsole: extract release appending into separate function
  net: netconsole: split send_msg_fragmented
  net: netconsole: fix a wrong warning

 drivers/net/netconsole.c | 207 +++++++++++++++++++++++++--------------
 1 file changed, 134 insertions(+), 73 deletions(-)

-- 
2.43.5

[PATCH net-next 1/9] net: netconsole: remove msg_ready variable

[Breno Leitao]

Variable msg_ready is useless, since it does not represent anything. Get
rid of it, using buf directly instead.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 01cf33fa7503..03150e513cb2 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1075,7 +1075,6 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 	const char *header, *body;
 	int offset = 0;
 	int header_len, body_len;
-	const char *msg_ready = msg;
 	const char *release;
 	int release_len = 0;
 	int userdata_len = 0;
@@ -1105,8 +1104,7 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 					     MAX_PRINT_CHUNK - msg_len,
 					     "%s", userdata);
 
-		msg_ready = buf;
-		netpoll_send_udp(&nt->np, msg_ready, msg_len);
+		netpoll_send_udp(&nt->np, buf, msg_len);
 		return;
 	}
 
-- 
2.43.5

Re: [PATCH net-next 1/9] net: netconsole: remove msg_ready variable

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:44AM -0700, Breno Leitao wrote:
> Variable msg_ready is useless, since it does not represent anything. Get
> rid of it, using buf directly instead.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 2/9] net: netconsole: split send_ext_msg_udp() function

[Breno Leitao]

The send_ext_msg_udp() function has become quite large, currently
spanning 102 lines. Its complexity, along with extensive pointer and
offset manipulation, makes it difficult to read and error-prone.

The function has evolved over time, and it’s now due for a refactor.

To improve readability and maintainability, isolate the case where no
message fragmentation occurs into a separate function, into a new
send_msg_no_fragmentation() function. This scenario covers about 95% of
the messages.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 45 +++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 03150e513cb2..0e43dacbd291 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1058,6 +1058,32 @@ static struct notifier_block netconsole_netdev_notifier = {
 	.notifier_call  = netconsole_netdev_event,
 };
 
+static void send_msg_no_fragmentation(struct netconsole_target *nt,
+				      const char *msg,
+				      const char *userdata,
+				      int msg_len,
+				      int release_len)
+{
+	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
+	const char *release;
+
+	if (release_len) {
+		release = init_utsname()->release;
+
+		scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
+		msg_len += release_len;
+	} else {
+		memcpy(buf, msg, msg_len);
+	}
+
+	if (userdata)
+		msg_len += scnprintf(&buf[msg_len],
+				     MAX_PRINT_CHUNK - msg_len,
+				     "%s", userdata);
+
+	netpoll_send_udp(&nt->np, buf, msg_len);
+}
+
 /**
  * send_ext_msg_udp - send extended log message to target
  * @nt: target to send message to
@@ -1090,23 +1116,8 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 		release_len = strlen(release) + 1;
 	}
 
-	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK) {
-		/* No fragmentation needed */
-		if (nt->release) {
-			scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
-			msg_len += release_len;
-		} else {
-			memcpy(buf, msg, msg_len);
-		}
-
-		if (userdata)
-			msg_len += scnprintf(&buf[msg_len],
-					     MAX_PRINT_CHUNK - msg_len,
-					     "%s", userdata);
-
-		netpoll_send_udp(&nt->np, buf, msg_len);
-		return;
-	}
+	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK)
+		return send_msg_no_fragmentation(nt, msg, userdata, msg_len, release_len);
 
 	/* need to insert extra header fields, detect header and body */
 	header = msg;
-- 
2.43.5

Re: [PATCH net-next 2/9] net: netconsole: split send_ext_msg_udp() function

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:45AM -0700, Breno Leitao wrote:
> The send_ext_msg_udp() function has become quite large, currently
> spanning 102 lines. Its complexity, along with extensive pointer and
> offset manipulation, makes it difficult to read and error-prone.
> 
> The function has evolved over time, and it’s now due for a refactor.
> 
> To improve readability and maintainability, isolate the case where no
> message fragmentation occurs into a separate function, into a new
> send_msg_no_fragmentation() function. This scenario covers about 95% of
> the messages.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Thanks,

The nit below aside this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> @@ -1090,23 +1116,8 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
>  		release_len = strlen(release) + 1;
>  	}
>  
> -	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK) {
> -		/* No fragmentation needed */
> -		if (nt->release) {
> -			scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
> -			msg_len += release_len;
> -		} else {
> -			memcpy(buf, msg, msg_len);
> -		}
> -
> -		if (userdata)
> -			msg_len += scnprintf(&buf[msg_len],
> -					     MAX_PRINT_CHUNK - msg_len,
> -					     "%s", userdata);
> -
> -		netpoll_send_udp(&nt->np, buf, msg_len);
> -		return;
> -	}
> +	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK)
> +		return send_msg_no_fragmentation(nt, msg, userdata, msg_len, release_len);

nit: This appears to be fixed in the following patch,
     but the above line could be wrapped here.

>  
>  	/* need to insert extra header fields, detect header and body */
>  	header = msg;
> -- 
> 2.43.5
> 

[PATCH net-next 3/9] net: netconsole: separate fragmented message handling in send_ext_msg

[Breno Leitao]

Following the previous change, where the non-fragmented case was moved
to its own function, this update introduces a new function called
send_msg_fragmented to specifically manage scenarios where message
fragmentation is required.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 129 ++++++++++++++++++++++-----------------
 1 file changed, 74 insertions(+), 55 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 0e43dacbd291..176ce6c616cb 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1058,66 +1058,20 @@ static struct notifier_block netconsole_netdev_notifier = {
 	.notifier_call  = netconsole_netdev_event,
 };
 
-static void send_msg_no_fragmentation(struct netconsole_target *nt,
-				      const char *msg,
-				      const char *userdata,
-				      int msg_len,
-				      int release_len)
-{
-	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
-	const char *release;
-
-	if (release_len) {
-		release = init_utsname()->release;
-
-		scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
-		msg_len += release_len;
-	} else {
-		memcpy(buf, msg, msg_len);
-	}
-
-	if (userdata)
-		msg_len += scnprintf(&buf[msg_len],
-				     MAX_PRINT_CHUNK - msg_len,
-				     "%s", userdata);
-
-	netpoll_send_udp(&nt->np, buf, msg_len);
-}
-
-/**
- * send_ext_msg_udp - send extended log message to target
- * @nt: target to send message to
- * @msg: extended log message to send
- * @msg_len: length of message
- *
- * Transfer extended log @msg to @nt.  If @msg is longer than
- * MAX_PRINT_CHUNK, it'll be split and transmitted in multiple chunks with
- * ncfrag header field added to identify them.
- */
-static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
-			     int msg_len)
+static void send_msg_fragmented(struct netconsole_target *nt,
+				const char *msg,
+				const char *userdata,
+				int msg_len,
+				int release_len)
 {
 	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
+	int offset = 0, userdata_len = 0;
 	const char *header, *body;
-	int offset = 0;
 	int header_len, body_len;
 	const char *release;
-	int release_len = 0;
-	int userdata_len = 0;
-	char *userdata = NULL;
-
-#ifdef CONFIG_NETCONSOLE_DYNAMIC
-	userdata = nt->userdata_complete;
-	userdata_len = nt->userdata_length;
-#endif
-
-	if (nt->release) {
-		release = init_utsname()->release;
-		release_len = strlen(release) + 1;
-	}
 
-	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK)
-		return send_msg_no_fragmentation(nt, msg, userdata, msg_len, release_len);
+	if (userdata)
+		userdata_len = nt->userdata_length;
 
 	/* need to insert extra header fields, detect header and body */
 	header = msg;
@@ -1133,11 +1087,18 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 	 * Transfer multiple chunks with the following extra header.
 	 * "ncfrag=<byte-offset>/<total-bytes>"
 	 */
-	if (nt->release)
+	if (release_len) {
+		release = init_utsname()->release;
 		scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
+	}
+
+	/* Copy the header into the buffer */
 	memcpy(buf + release_len, header, header_len);
 	header_len += release_len;
 
+	/* for now on, the header will be persisted, and the body
+	 * will be replaced
+	 */
 	while (offset < body_len + userdata_len) {
 		int this_header = header_len;
 		int this_offset = 0;
@@ -1183,6 +1144,64 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 	}
 }
 
+static void send_msg_no_fragmentation(struct netconsole_target *nt,
+				      const char *msg,
+				      const char *userdata,
+				      int msg_len,
+				      int release_len)
+{
+	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
+	const char *release;
+
+	if (release_len) {
+		release = init_utsname()->release;
+
+		scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg);
+		msg_len += release_len;
+	} else {
+		memcpy(buf, msg, msg_len);
+	}
+
+	if (userdata)
+		msg_len += scnprintf(&buf[msg_len],
+				     MAX_PRINT_CHUNK - msg_len,
+				     "%s", userdata);
+
+	netpoll_send_udp(&nt->np, buf, msg_len);
+}
+
+/**
+ * send_ext_msg_udp - send extended log message to target
+ * @nt: target to send message to
+ * @msg: extended log message to send
+ * @msg_len: length of message
+ *
+ * Transfer extended log @msg to @nt.  If @msg is longer than
+ * MAX_PRINT_CHUNK, it'll be split and transmitted in multiple chunks with
+ * ncfrag header field added to identify them.
+ */
+static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
+			     int msg_len)
+{
+	char *userdata = NULL;
+	int userdata_len = 0;
+	int release_len = 0;
+
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+	userdata = nt->userdata_complete;
+	userdata_len = nt->userdata_length;
+#endif
+
+	if (nt->release)
+		release_len = strlen(init_utsname()->release) + 1;
+
+	if (msg_len + release_len + userdata_len <= MAX_PRINT_CHUNK)
+		return send_msg_no_fragmentation(nt, msg, userdata, msg_len,
+						 release_len);
+
+	return send_msg_fragmented(nt, msg, userdata, msg_len, release_len);
+}
+
 static void write_ext_msg(struct console *con, const char *msg,
 			  unsigned int len)
 {
-- 
2.43.5

Re: [PATCH net-next 3/9] net: netconsole: separate fragmented message handling in send_ext_msg

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:46AM -0700, Breno Leitao wrote:
> Following the previous change, where the non-fragmented case was moved
> to its own function, this update introduces a new function called
> send_msg_fragmented to specifically manage scenarios where message
> fragmentation is required.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Due to tooling the diff below seems to more verbose than the change
warrants. Perhaps some diff flags would alleviate this, but anyone viewing
the patch using git with default flags, would see what is below anyway.

So I wonder if you could consider moving send_msg_fragmented()
to above send_msg_no_fragmentation(). Locally this lead to an entirely
more reasonable diff to review.

I did review this change using that technique, and it looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 3/9] net: netconsole: separate fragmented message handling in send_ext_msg

[Breno Leitao]

On Wed, Sep 04, 2024 at 11:59:20AM +0100, Simon Horman wrote:
> On Tue, Sep 03, 2024 at 07:07:46AM -0700, Breno Leitao wrote:
> > Following the previous change, where the non-fragmented case was moved
> > to its own function, this update introduces a new function called
> > send_msg_fragmented to specifically manage scenarios where message
> > fragmentation is required.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Due to tooling the diff below seems to more verbose than the change
> warrants. Perhaps some diff flags would alleviate this, but anyone viewing
> the patch using git with default flags, would see what is below anyway.
> 
> So I wonder if you could consider moving send_msg_fragmented()
> to above send_msg_no_fragmentation(). Locally this lead to an entirely
> more reasonable diff to review.

I agree. Let me move the functions around aiming to generate an
easy-to-review diff.

Thanks for the feedback.

[PATCH net-next 4/9] net: netconsole: rename body to msg_body

[Breno Leitao]

With the introduction of the userdata concept, the term body has become
ambiguous and less intuitive.

To improve clarity, body is renamed to msg_body, making it clear that
the body is not the only content following the header.

In an upcoming patch, the term body_len will also be revised for further
clarity.

The current packet structure is as follows:

	release, header, body, [msg_body + userdata]

Here, [msg_body + userdata] collectively forms what is currently
referred to as "body." This renaming helps to distinguish and better
understand each component of the packet.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 176ce6c616cb..0d924fba5814 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1066,22 +1066,22 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 {
 	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
 	int offset = 0, userdata_len = 0;
-	const char *header, *body;
-	int header_len, body_len;
+	const char *header, *msgbody;
+	int header_len, msgbody_len;
 	const char *release;
 
 	if (userdata)
 		userdata_len = nt->userdata_length;
 
-	/* need to insert extra header fields, detect header and body */
+	/* need to insert extra header fields, detect header and msgbody */
 	header = msg;
-	body = memchr(msg, ';', msg_len);
-	if (WARN_ON_ONCE(!body))
+	msgbody = memchr(msg, ';', msg_len);
+	if (WARN_ON_ONCE(!msgbody))
 		return;
 
-	header_len = body - header;
-	body_len = msg_len - header_len - 1;
-	body++;
+	header_len = msgbody - header;
+	msgbody_len = msg_len - header_len - 1;
+	msgbody++;
 
 	/*
 	 * Transfer multiple chunks with the following extra header.
@@ -1096,10 +1096,10 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	memcpy(buf + release_len, header, header_len);
 	header_len += release_len;
 
-	/* for now on, the header will be persisted, and the body
+	/* for now on, the header will be persisted, and the msgbody
 	 * will be replaced
 	 */
-	while (offset < body_len + userdata_len) {
+	while (offset < msgbody_len + userdata_len) {
 		int this_header = header_len;
 		int this_offset = 0;
 		int this_chunk = 0;
@@ -1107,23 +1107,23 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 		this_header += scnprintf(buf + this_header,
 					 sizeof(buf) - this_header,
 					 ",ncfrag=%d/%d;", offset,
-					 body_len + userdata_len);
+					 msgbody_len + userdata_len);
 
-		/* Not all body data has been written yet */
-		if (offset < body_len) {
-			this_chunk = min(body_len - offset,
+		/* Not all msgbody data has been written yet */
+		if (offset < msgbody_len) {
+			this_chunk = min(msgbody_len - offset,
 					 MAX_PRINT_CHUNK - this_header);
 			if (WARN_ON_ONCE(this_chunk <= 0))
 				return;
-			memcpy(buf + this_header, body + offset, this_chunk);
+			memcpy(buf + this_header, msgbody + offset, this_chunk);
 			this_offset += this_chunk;
 		}
-		/* Body is fully written and there is pending userdata to write,
+		/* Msg body is fully written and there is pending userdata to write,
 		 * append userdata in this chunk
 		 */
-		if (offset + this_offset >= body_len &&
-		    offset + this_offset < userdata_len + body_len) {
-			int sent_userdata = (offset + this_offset) - body_len;
+		if (offset + this_offset >= msgbody_len &&
+		    offset + this_offset < userdata_len + msgbody_len) {
+			int sent_userdata = (offset + this_offset) - msgbody_len;
 			int preceding_bytes = this_chunk + this_header;
 
 			if (WARN_ON_ONCE(sent_userdata < 0))
-- 
2.43.5

Re: [PATCH net-next 4/9] net: netconsole: rename body to msg_body

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:47AM -0700, Breno Leitao wrote:
> With the introduction of the userdata concept, the term body has become
> ambiguous and less intuitive.
> 
> To improve clarity, body is renamed to msg_body, making it clear that

nit: the patch uses msgbody

> the body is not the only content following the header.
> 
> In an upcoming patch, the term body_len will also be revised for further
> clarity.
> 
> The current packet structure is as follows:
> 
> 	release, header, body, [msg_body + userdata]
> 
> Here, [msg_body + userdata] collectively forms what is currently
> referred to as "body." This renaming helps to distinguish and better
> understand each component of the packet.

Thanks, IMHO, clear terminology is very important:
it's hard to discuss what you can't clearly name.

> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Code changes are a mechanical update and look good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

[PATCH net-next 5/9] net: netconsole: introduce variable to track body length

[Breno Leitao]

This new variable tracks the total length of the data to be sent,
encompassing both the message body (msgbody) and userdata, which is
collectively called body.

By explicitly defining body_len, the code becomes clearer and easier to
reason about, simplifying offset calculations and improving overall
readability of the function.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 0d924fba5814..22ccd9aa016a 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1064,10 +1064,10 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 				int msg_len,
 				int release_len)
 {
+	int header_len, msgbody_len, body_len;
 	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
 	int offset = 0, userdata_len = 0;
 	const char *header, *msgbody;
-	int header_len, msgbody_len;
 	const char *release;
 
 	if (userdata)
@@ -1096,10 +1096,11 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	memcpy(buf + release_len, header, header_len);
 	header_len += release_len;
 
+	body_len = msgbody_len + userdata_len;
 	/* for now on, the header will be persisted, and the msgbody
 	 * will be replaced
 	 */
-	while (offset < msgbody_len + userdata_len) {
+	while (offset < body_len) {
 		int this_header = header_len;
 		int this_offset = 0;
 		int this_chunk = 0;
@@ -1107,7 +1108,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 		this_header += scnprintf(buf + this_header,
 					 sizeof(buf) - this_header,
 					 ",ncfrag=%d/%d;", offset,
-					 msgbody_len + userdata_len);
+					 body_len);
 
 		/* Not all msgbody data has been written yet */
 		if (offset < msgbody_len) {
@@ -1122,7 +1123,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 		 * append userdata in this chunk
 		 */
 		if (offset + this_offset >= msgbody_len &&
-		    offset + this_offset < userdata_len + msgbody_len) {
+		    offset + this_offset < body_len) {
 			int sent_userdata = (offset + this_offset) - msgbody_len;
 			int preceding_bytes = this_chunk + this_header;
 
-- 
2.43.5

Re: [PATCH net-next 5/9] net: netconsole: introduce variable to track body length

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:48AM -0700, Breno Leitao wrote:
> This new variable tracks the total length of the data to be sent,
> encompassing both the message body (msgbody) and userdata, which is
> collectively called body.
> 
> By explicitly defining body_len, the code becomes clearer and easier to
> reason about, simplifying offset calculations and improving overall
> readability of the function.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Thanks,

I agree that this improves the clarify of the code.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 6/9] net: netconsole: track explicitly if msgbody was written to buffer

[Breno Leitao]

The current check to determine if the message body was fully sent is
difficult to follow. To improve clarity, introduce a variable that
explicitly tracks whether the message body (msgbody) has been completely
sent, indicating when it's time to begin sending userdata.

Additionally, add comments to make the code more understandable for
others who may work with it.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 22ccd9aa016a..c8a23a7684e5 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1102,6 +1102,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	 */
 	while (offset < body_len) {
 		int this_header = header_len;
+		bool msgbody_written = false;
 		int this_offset = 0;
 		int this_chunk = 0;
 
@@ -1119,12 +1120,22 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 			memcpy(buf + this_header, msgbody + offset, this_chunk);
 			this_offset += this_chunk;
 		}
+
+		if (offset + this_offset >= msgbody_len)
+			/* msgbody was finally written, either in the previous messages
+			 * and/or in the current buf. Time to write the userdata.
+			 */
+			msgbody_written = true;
+
 		/* Msg body is fully written and there is pending userdata to write,
 		 * append userdata in this chunk
 		 */
-		if (offset + this_offset >= msgbody_len &&
-		    offset + this_offset < body_len) {
+		if (msgbody_written && offset + this_offset < body_len) {
+			/* Track how much user data was already sent. First time here, sent_userdata
+			 * is zero
+			 */
 			int sent_userdata = (offset + this_offset) - msgbody_len;
+			/* offset of bytes used in current buf */
 			int preceding_bytes = this_chunk + this_header;
 
 			if (WARN_ON_ONCE(sent_userdata < 0))
-- 
2.43.5

Re: [PATCH net-next 6/9] net: netconsole: track explicitly if msgbody was written to buffer

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:49AM -0700, Breno Leitao wrote:
> The current check to determine if the message body was fully sent is
> difficult to follow. To improve clarity, introduce a variable that
> explicitly tracks whether the message body (msgbody) has been completely
> sent, indicating when it's time to begin sending userdata.
> 
> Additionally, add comments to make the code more understandable for
> others who may work with it.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Thanks,

The nit below notwithstanding this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/netconsole.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 22ccd9aa016a..c8a23a7684e5 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1102,6 +1102,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
>  	 */
>  	while (offset < body_len) {
>  		int this_header = header_len;
> +		bool msgbody_written = false;
>  		int this_offset = 0;
>  		int this_chunk = 0;
>  
> @@ -1119,12 +1120,22 @@ static void send_msg_fragmented(struct netconsole_target *nt,
>  			memcpy(buf + this_header, msgbody + offset, this_chunk);
>  			this_offset += this_chunk;
>  		}
> +
> +		if (offset + this_offset >= msgbody_len)
> +			/* msgbody was finally written, either in the previous messages
> +			 * and/or in the current buf. Time to write the userdata.
> +			 */

Please consider keeping comments <= 80 columns wide.
Likewise in other patches of this series.

checkpatch can be run with an option to check for this.

> +			msgbody_written = true;
> +
>  		/* Msg body is fully written and there is pending userdata to write,
>  		 * append userdata in this chunk
>  		 */
> -		if (offset + this_offset >= msgbody_len &&
> -		    offset + this_offset < body_len) {
> +		if (msgbody_written && offset + this_offset < body_len) {
> +			/* Track how much user data was already sent. First time here, sent_userdata
> +			 * is zero
> +			 */
>  			int sent_userdata = (offset + this_offset) - msgbody_len;
> +			/* offset of bytes used in current buf */
>  			int preceding_bytes = this_chunk + this_header;
>  
>  			if (WARN_ON_ONCE(sent_userdata < 0))
> -- 
> 2.43.5
> 

Re: [PATCH net-next 6/9] net: netconsole: track explicitly if msgbody was written to buffer

[Breno Leitao]

On Wed, Sep 04, 2024 at 12:07:26PM +0100, Simon Horman wrote:
> On Tue, Sep 03, 2024 at 07:07:49AM -0700, Breno Leitao wrote:
> > The current check to determine if the message body was fully sent is
> > difficult to follow. To improve clarity, introduce a variable that
> > explicitly tracks whether the message body (msgbody) has been completely
> > sent, indicating when it's time to begin sending userdata.
> > 
> > Additionally, add comments to make the code more understandable for
> > others who may work with it.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Thanks,
> 
> The nit below notwithstanding this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> > ---
> >  drivers/net/netconsole.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 22ccd9aa016a..c8a23a7684e5 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1102,6 +1102,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
> >  	 */
> >  	while (offset < body_len) {
> >  		int this_header = header_len;
> > +		bool msgbody_written = false;
> >  		int this_offset = 0;
> >  		int this_chunk = 0;
> >  
> > @@ -1119,12 +1120,22 @@ static void send_msg_fragmented(struct netconsole_target *nt,
> >  			memcpy(buf + this_header, msgbody + offset, this_chunk);
> >  			this_offset += this_chunk;
> >  		}
> > +
> > +		if (offset + this_offset >= msgbody_len)
> > +			/* msgbody was finally written, either in the previous messages
> > +			 * and/or in the current buf. Time to write the userdata.
> > +			 */
> 
> Please consider keeping comments <= 80 columns wide.
> Likewise in other patches of this series.
> 
> checkpatch can be run with an option to check for this.

Thanks for the heads-up. I've just added `--max-line-length=80` to my
checkpatch by default

Thanks

[PATCH net-next 7/9] net: netconsole: extract release appending into separate function

[Breno Leitao]

Refactor the code by extracting the logic for appending the
release into the buffer into a separate function.

The goal is to reduce the size of send_msg_fragmented() and improve
code readability.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index c8a23a7684e5..be23def330e9 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1058,6 +1058,14 @@ static struct notifier_block netconsole_netdev_notifier = {
 	.notifier_call  = netconsole_netdev_event,
 };
 
+static void append_release(char *buf)
+{
+	const char *release;
+
+	release = init_utsname()->release;
+	scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
+}
+
 static void send_msg_fragmented(struct netconsole_target *nt,
 				const char *msg,
 				const char *userdata,
@@ -1068,7 +1076,6 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
 	int offset = 0, userdata_len = 0;
 	const char *header, *msgbody;
-	const char *release;
 
 	if (userdata)
 		userdata_len = nt->userdata_length;
@@ -1087,10 +1094,8 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	 * Transfer multiple chunks with the following extra header.
 	 * "ncfrag=<byte-offset>/<total-bytes>"
 	 */
-	if (release_len) {
-		release = init_utsname()->release;
-		scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
-	}
+	if (release_len)
+		append_release(buf);
 
 	/* Copy the header into the buffer */
 	memcpy(buf + release_len, header, header_len);
-- 
2.43.5

Re: [PATCH net-next 7/9] net: netconsole: extract release appending into separate function

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:50AM -0700, Breno Leitao wrote:
> Refactor the code by extracting the logic for appending the
> release into the buffer into a separate function.
> 
> The goal is to reduce the size of send_msg_fragmented() and improve
> code readability.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Thanks,

I agree this improves readability.

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next 8/9] net: netconsole: split send_msg_fragmented

[Breno Leitao]

Refactor the send_msg_fragmented() function by extracting the logic for
sending the message body into a new function called
send_fragmented_body().

Now, send_msg_fragmented() handles appending the release and header, and
then delegates the task of sending the body to send_fragmented_body().

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 85 +++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 37 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index be23def330e9..81d7d2b09988 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1066,45 +1066,21 @@ static void append_release(char *buf)
 	scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
 }
 
-static void send_msg_fragmented(struct netconsole_target *nt,
-				const char *msg,
-				const char *userdata,
-				int msg_len,
-				int release_len)
+static void send_fragmented_body(struct netconsole_target *nt, char *buf,
+				 const char *msgbody, int header_len,
+				 int msgbody_len)
 {
-	int header_len, msgbody_len, body_len;
-	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
-	int offset = 0, userdata_len = 0;
-	const char *header, *msgbody;
-
-	if (userdata)
-		userdata_len = nt->userdata_length;
-
-	/* need to insert extra header fields, detect header and msgbody */
-	header = msg;
-	msgbody = memchr(msg, ';', msg_len);
-	if (WARN_ON_ONCE(!msgbody))
-		return;
-
-	header_len = msgbody - header;
-	msgbody_len = msg_len - header_len - 1;
-	msgbody++;
-
-	/*
-	 * Transfer multiple chunks with the following extra header.
-	 * "ncfrag=<byte-offset>/<total-bytes>"
-	 */
-	if (release_len)
-		append_release(buf);
+	int body_len, offset = 0;
+	const char *userdata = NULL;
+	int userdata_len = 0;
 
-	/* Copy the header into the buffer */
-	memcpy(buf + release_len, header, header_len);
-	header_len += release_len;
+#ifdef CONFIG_NETCONSOLE_DYNAMIC
+	userdata = nt->userdata_complete;
+	userdata_len = nt->userdata_length;
+#endif
 
 	body_len = msgbody_len + userdata_len;
-	/* for now on, the header will be persisted, and the msgbody
-	 * will be replaced
-	 */
+
 	while (offset < body_len) {
 		int this_header = header_len;
 		bool msgbody_written = false;
@@ -1112,7 +1088,7 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 		int this_chunk = 0;
 
 		this_header += scnprintf(buf + this_header,
-					 sizeof(buf) - this_header,
+					 MAX_PRINT_CHUNK - this_header,
 					 ",ncfrag=%d/%d;", offset,
 					 body_len);
 
@@ -1161,6 +1137,41 @@ static void send_msg_fragmented(struct netconsole_target *nt,
 	}
 }
 
+static void send_msg_fragmented(struct netconsole_target *nt,
+				const char *msg,
+				int msg_len,
+				int release_len)
+{
+	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
+	int header_len, msgbody_len;
+	const char *msgbody;
+
+	/* need to insert extra header fields, detect header and msgbody */
+	msgbody = memchr(msg, ';', msg_len);
+	if (WARN_ON_ONCE(!msgbody))
+		return;
+
+	header_len = msgbody - msg;
+	msgbody_len = msg_len - header_len - 1;
+	msgbody++;
+
+	/*
+	 * Transfer multiple chunks with the following extra header.
+	 * "ncfrag=<byte-offset>/<total-bytes>"
+	 */
+	if (release_len)
+		append_release(buf);
+
+	/* Copy the header into the buffer */
+	memcpy(buf + release_len, msg, header_len);
+	header_len += release_len;
+
+	/* for now on, the header will be persisted, and the msgbody
+	 * will be replaced
+	 */
+	send_fragmented_body(nt, buf, msgbody, header_len, msgbody_len);
+}
+
 static void send_msg_no_fragmentation(struct netconsole_target *nt,
 				      const char *msg,
 				      const char *userdata,
@@ -1216,7 +1227,7 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 		return send_msg_no_fragmentation(nt, msg, userdata, msg_len,
 						 release_len);
 
-	return send_msg_fragmented(nt, msg, userdata, msg_len, release_len);
+	return send_msg_fragmented(nt, msg, msg_len, release_len);
 }
 
 static void write_ext_msg(struct console *con, const char *msg,
-- 
2.43.5

Re: [PATCH net-next 8/9] net: netconsole: split send_msg_fragmented

[Simon Horman]

On Tue, Sep 03, 2024 at 07:07:51AM -0700, Breno Leitao wrote:
> Refactor the send_msg_fragmented() function by extracting the logic for
> sending the message body into a new function called
> send_fragmented_body().
> 
> Now, send_msg_fragmented() handles appending the release and header, and
> then delegates the task of sending the body to send_fragmented_body().

I think it would be good to expand a bit on why here.

> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 85 +++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index be23def330e9..81d7d2b09988 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1066,45 +1066,21 @@ static void append_release(char *buf)
>  	scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
>  }
>  
> -static void send_msg_fragmented(struct netconsole_target *nt,
> -				const char *msg,
> -				const char *userdata,
> -				int msg_len,
> -				int release_len)
> +static void send_fragmented_body(struct netconsole_target *nt, char *buf,
> +				 const char *msgbody, int header_len,
> +				 int msgbody_len)
>  {
> -	int header_len, msgbody_len, body_len;
> -	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
> -	int offset = 0, userdata_len = 0;
> -	const char *header, *msgbody;
> -
> -	if (userdata)
> -		userdata_len = nt->userdata_length;
> -
> -	/* need to insert extra header fields, detect header and msgbody */
> -	header = msg;
> -	msgbody = memchr(msg, ';', msg_len);
> -	if (WARN_ON_ONCE(!msgbody))
> -		return;
> -
> -	header_len = msgbody - header;
> -	msgbody_len = msg_len - header_len - 1;
> -	msgbody++;
> -
> -	/*
> -	 * Transfer multiple chunks with the following extra header.
> -	 * "ncfrag=<byte-offset>/<total-bytes>"
> -	 */
> -	if (release_len)
> -		append_release(buf);
> +	int body_len, offset = 0;
> +	const char *userdata = NULL;
> +	int userdata_len = 0;
>  
> -	/* Copy the header into the buffer */
> -	memcpy(buf + release_len, header, header_len);
> -	header_len += release_len;
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +	userdata = nt->userdata_complete;
> +	userdata_len = nt->userdata_length;
> +#endif

I think that dropping the userdata parameter of send_msg_fragmented() ought
to part of an earlier patch or separate patch. It doesn't seem strictly
related to this patch.

>  
>  	body_len = msgbody_len + userdata_len;
> -	/* for now on, the header will be persisted, and the msgbody
> -	 * will be replaced
> -	 */
> +
>  	while (offset < body_len) {
>  		int this_header = header_len;
>  		bool msgbody_written = false;

...

> @@ -1161,6 +1137,41 @@ static void send_msg_fragmented(struct netconsole_target *nt,
>  	}
>  }
>  
> +static void send_msg_fragmented(struct netconsole_target *nt,
> +				const char *msg,
> +				int msg_len,
> +				int release_len)

...

Re: [PATCH net-next 8/9] net: netconsole: split send_msg_fragmented

[Breno Leitao]

On Wed, Sep 04, 2024 at 12:16:36PM +0100, Simon Horman wrote:
> On Tue, Sep 03, 2024 at 07:07:51AM -0700, Breno Leitao wrote:
> > Refactor the send_msg_fragmented() function by extracting the logic for
> > sending the message body into a new function called
> > send_fragmented_body().
> > 
> > Now, send_msg_fragmented() handles appending the release and header, and
> > then delegates the task of sending the body to send_fragmented_body().
> 
> I think it would be good to expand a bit on why here.
> 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/net/netconsole.c | 85 +++++++++++++++++++++++-----------------
> >  1 file changed, 48 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index be23def330e9..81d7d2b09988 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1066,45 +1066,21 @@ static void append_release(char *buf)
> >  	scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release);
> >  }
> >  
> > -static void send_msg_fragmented(struct netconsole_target *nt,
> > -				const char *msg,
> > -				const char *userdata,
> > -				int msg_len,
> > -				int release_len)
> > +static void send_fragmented_body(struct netconsole_target *nt, char *buf,
> > +				 const char *msgbody, int header_len,
> > +				 int msgbody_len)
> >  {
> > -	int header_len, msgbody_len, body_len;
> > -	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
> > -	int offset = 0, userdata_len = 0;
> > -	const char *header, *msgbody;
> > -
> > -	if (userdata)
> > -		userdata_len = nt->userdata_length;
> > -
> > -	/* need to insert extra header fields, detect header and msgbody */
> > -	header = msg;
> > -	msgbody = memchr(msg, ';', msg_len);
> > -	if (WARN_ON_ONCE(!msgbody))
> > -		return;
> > -
> > -	header_len = msgbody - header;
> > -	msgbody_len = msg_len - header_len - 1;
> > -	msgbody++;
> > -
> > -	/*
> > -	 * Transfer multiple chunks with the following extra header.
> > -	 * "ncfrag=<byte-offset>/<total-bytes>"
> > -	 */
> > -	if (release_len)
> > -		append_release(buf);
> > +	int body_len, offset = 0;
> > +	const char *userdata = NULL;
> > +	int userdata_len = 0;
> >  
> > -	/* Copy the header into the buffer */
> > -	memcpy(buf + release_len, header, header_len);
> > -	header_len += release_len;
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +	userdata = nt->userdata_complete;
> > +	userdata_len = nt->userdata_length;
> > +#endif
> 
> I think that dropping the userdata parameter of send_msg_fragmented() ought
> to part of an earlier patch or separate patch. It doesn't seem strictly
> related to this patch.

I agree with you. Let me separate it in a different patch, then.

Thanks for the review,
--breno

[PATCH net-next 9/9] net: netconsole: Fix a wrong warning

[Breno Leitao]

A warning is triggered when there is insufficient space in the buffer
for userdata. However, this is not an issue since userdata will be sent
in the next iteration.

Current warning message:

    ------------[ cut here ]------------
     WARNING: CPU: 13 PID: 3013042 at drivers/net/netconsole.c:1122 write_ext_msg+0x3b6/0x3d0
      ? write_ext_msg+0x3b6/0x3d0
      console_flush_all+0x1e9/0x330

The code incorrectly issues a warning when this_chunk is zero, which is
a valid scenario. The warning should only be triggered when this_chunk
is negative.

Signed-off-by: Breno Leitao <leitao@debian.org>
Fixes: 1ec9daf95093 ("net: netconsole: append userdata to fragmented netconsole messages")
---
 drivers/net/netconsole.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 81d7d2b09988..83662a28dc00 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1124,8 +1124,13 @@ static void send_fragmented_body(struct netconsole_target *nt, char *buf,
 
 			this_chunk = min(userdata_len - sent_userdata,
 					 MAX_PRINT_CHUNK - preceding_bytes);
-			if (WARN_ON_ONCE(this_chunk <= 0))
+			if (WARN_ON_ONCE(this_chunk < 0))
+				/* this_chunk could be zero if all the previous message used
+				 * all the buffer. This is not a problem, userdata will be sent
+				 * in the next iteration
+				 */
 				return;
+
 			memcpy(buf + this_header + this_offset,
 			       userdata + sent_userdata,
 			       this_chunk);
-- 
2.43.5