Page MenuHomeFreeBSD

Fix netgdb double ack, print proxy address
ClosedPublic

Authored by john.reimer_dell.com on May 11 2023, 2:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 21, 10:24 PM
Unknown Object (File)
Mon, Sep 11, 1:34 PM
Unknown Object (File)
Aug 14 2023, 11:06 PM
Unknown Object (File)
Aug 14 2023, 1:01 PM
Unknown Object (File)
Jul 10 2023, 10:19 AM
Unknown Object (File)
Jun 21 2023, 1:58 PM
Unknown Object (File)
May 27 2023, 5:14 PM
Unknown Object (File)
May 27 2023, 4:36 PM

Details

Summary
  • Fixes netgdb's double ack
  • Moving ack responsibility to debugnet, decision to ack made by netdump/netgdb
  • Finish responsibility moved to debugnet, new finish handler
  • netgdb now prints the address to connect to in case the user doesn't have access to the proxy machine

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

john.reimer_dell.com retitled this revision from Debugnet fixes to Fix netgdb double ack, print proxy address.May 11 2023, 2:40 PM
john.reimer_dell.com edited the summary of this revision. (Show Details)
john.reimer_dell.com edited the summary of this revision. (Show Details)
bdrewery added a subscriber: bdrewery.

I mention "style" which is from style(9) man style. Accepted pending style fixes.

sys/gdb/netgdb.c
131

Style return (ENOBUFS);

154

I was going to say style requires a blank newline here since the are no variable declarations but I see it is "optional" now in style(9). I'm leaving the comment here in case others didn't know that yet.

375–377

Style requires putting variable declarations at the top of the block and possibly not making function calls depending on the function complexity. Personally I am okay with the function call here but others may not be.
The const ones here may be OKAY but the proxy_buf array probably needs to go to the top.

When declaring variables in functions declare them sorted by size, then in alphabetical order; multiple ones per line are okay.  If a line overflows reuse the type keyword.  Variables may be initialized where declared especially when they are
constant for the rest of the scope.  Declarations may be in any block, but must be placed before statements.  Calls to complicated functions should be avoided when initializing variables.
sys/net/debugnet.c
123

Style return (&pcb->dp_server);

131

Style return (pcb->dp_server_port);

404–410

Trivial but maybe we should initialize after checking the packet size since the variables won't be used in that case?

433

Style says unless error is boolean it should be compared to a value. != 0.

This revision is now accepted and ready to land.May 11 2023, 8:55 PM
This revision now requires review to proceed.May 12 2023, 2:09 PM

Fixes netgdb's double ack
Moving ack responsibility to debugnet, decision to ack made by netdump/netgdb

I have zero context for any of this, so some explanation of the problem and its solution would be appreciated.

I have zero context for any of this, so some explanation of the problem and its solution would be appreciated.

Right now, the rx datapath acks incoming packets twice, once in debugnet (debugnet.c line 398) and then again when the mbuf is passed to netgdb (netgdb.c line 157). Both places make some sense, as debugnet is handling the protocol while netgdb needs to determine whether or not to ack. I decided the best way to handle this would be to have debugnet conditionally ack on a return value from netgdb_rx. This does reorder the process a bit, from ack -> ack -> process packet to process packet -> ack. There is some grounds for a should_ack function (should_ack -> ack -> process_packet). My implementation moves all the protocol-specific code from netgdb to debugnet, and netgdb's return value informs debugnet on how to proceed.

I have zero context for any of this, so some explanation of the problem and its solution would be appreciated.

Right now, the rx datapath acks incoming packets twice, once in debugnet (debugnet.c line 398) and then again when the mbuf is passed to netgdb (netgdb.c line 157). Both places make some sense, as debugnet is handling the protocol while netgdb needs to determine whether or not to ack. I decided the best way to handle this would be to have debugnet conditionally ack on a return value from netgdb_rx. This does reorder the process a bit, from ack -> ack -> process packet to process packet -> ack. There is some grounds for a should_ack function (should_ack -> ack -> process_packet). My implementation moves all the protocol-specific code from netgdb to debugnet, and netgdb's return value informs debugnet on how to proceed.

Thanks for the explanation.

The patch seems ok to me, my comments are for minor issues.

sys/gdb/netgdb.c
117

m is no longer an out-parameter; this part of the comment is stale.

sys/net/debugnet.c
436

It'd be nice to print the error number here.

sys/net/debugnet.h
216

debugnet_get_server_addr() seems like a better name.

Addressed review comments.

sys/gdb/netgdb.c
117

The comment still doesn't make sense. The handler should not free the mbuf chain.

The last upload is just a patch which modifies the comment. With phabricator you generally need to upload the whole patch each time you revise it. But assuming it's otherwise unchanged, I think this is ok.

This revision is now accepted and ready to land.May 25 2023, 8:29 PM

Reuploading the full set of changes

This revision now requires review to proceed.May 26 2023, 7:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 27 2023, 4:36 PM
This revision was automatically updated to reflect the committed changes.