diff mbox series

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

Message ID 20240903140757.2802765-4-leitao@debian.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series netconsole refactoring and warning fix | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 157 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-09-04--15-00 (tests: 718)

Commit Message

Breno Leitao Sept. 3, 2024, 2:07 p.m. UTC
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(-)

Comments

Simon Horman Sept. 4, 2024, 10:59 a.m. UTC | #1
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>
Breno Leitao Sept. 6, 2024, 8:33 a.m. UTC | #2
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.
diff mbox series

Patch

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)
 {