On Fri, Nov 7, 2025 at 1:23 PM Breno Leitao leitao@debian.org wrote:
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 0a8ba7c4bc9d..e780c884db83 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -50,7 +50,8 @@ MODULE_LICENSE("GPL"); /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */ #define MAX_EXTRADATA_NAME_LEN (MAX_EXTRADATA_ENTRY_LEN - \ MAX_EXTRADATA_VALUE_LEN - 3) -#define MAX_EXTRADATA_ITEMS 16 +#define MAX_USERDATA_ITEMS 16
Do we still need to have MAX_USERDATA_ITEMS cap with your new approach?
That is a good point. I did think about this and ended up deciding to keep a limit as a safety measure against userspace creating a boatload of items until we run out of memory.
+#define MAX_SYSDATA_ITEMS 4
Can we have this one inside enum sysdata_feature?
Something as:
enum sysdata_feature { SYSDATA_CPU_NR = BIT(0), SYSDATA_TASKNAME = BIT(1), SYSDATA_RELEASE = BIT(2), SYSDATA_MSGID = BIT(3), MAX_SYSDATA_ITEMS = 4 /* Sentinel: highest bit position */ };
Sure, I will do this in v2.
@@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset) {
/* Append cpu=%d at extradata_complete after userdata str */return scnprintf(&nt->extradata_complete[offset],
return scnprintf(&nt->sysdata[offset], MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",This is confusing. It is writing to sysdata but checking extradata entry len.
My understanding is that extradata is a way to refer to both userdata and sysdata, and MAX_EXTRADATA_ENTRY_LEN is the maximum size for both (and the arithmetic used to define MAX_EXTRADATA_VALUE_LEN and MAX_EXTRADATA_NAME_LEN also applies to both). Do you want to define separate maximum sizes for userdata items and sysdata items?
@@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt, memcpy(nt->buf, msg, msg_len); }
if (extradata)msg_len += scnprintf(&nt->buf[msg_len],MAX_PRINT_CHUNK - msg_len,"%s", extradata);+#ifdef CONFIG_NETCONSOLE_DYNAMIC
msg_len += scnprintf(&nt->buf[msg_len],MAX_PRINT_CHUNK - msg_len,"%s%s", nt->userdata, nt->sysdata);+#endif
I am not sure I like this ifdef in here. Can you if userdata or sysdata are valid, and then scnprintf() instead of using ifdef?
OK, will do that in v2.
@@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt, msgbody_offset += this_chunk; this_offset += this_chunk;
/* after msgbody, append extradata */this_chunk = min(extradata_len - extradata_offset,
/* after msgbody, append userdata */this_chunk = min(userdata_len - userdata_offset,Please assign this "userdata_len - userdata_offset" to a variable and give it a name, so, it help us to reason about the code.
What about:
int data_remaining = userdata_len - userdata_offset; int buffer_remaining = MAX_PRINT_CHUNK - this_header - this_offset; this_chunk = min(data_remaining, buffer_remaining);
MAX_PRINT_CHUNK - this_header - this_offset); memcpy(nt->buf + this_header + this_offset,
extradata + extradata_offset, this_chunk);extradata_offset += this_chunk;
userdata + userdata_offset, this_chunk);userdata_offset += this_chunk;this_offset += this_chunk;/* after userdata, append sysdata */this_chunk = min(sysdata_len - sysdata_offset,MAX_PRINT_CHUNK - this_header - this_offset);memcpy(nt->buf + this_header + this_offset,sysdata + sysdata_offset, this_chunk);
I realize we have the same NULL pointer arithmetic problem here. I will fix it by checking if sysdata or userdata is NULL.
s/this_header/this_header_offset?
I just realized that this_header is not even necessary. I can simply add header_len to this_offset and get rid of this variable altogether.
Now that you are touching this code, please review these variable to keep them named correct.
Maybe adding _ptr for pointer, and _offset for offsets and _len for lenghts?
Once I get rid of this_header and add the _ptr suffix, I think it will be much clearer.
Also, I find offset and this_offset confusing. What about replacing by msg_offset and buffer_offset ?
Thank you for your work reasong about all of this!
Thanks for the review!