| Message ID | 20240903140757.2802765-9-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netconsole refactoring and warning fix | expand |
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) ...
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
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,
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(-)