| Message ID | 20240903140757.2802765-7-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: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 >
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
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))
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(-)