- 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
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 51692 Build 48583: arc lint + arc unit
Event Timeline
Comment Actions
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. 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 ↗ | (On Diff #121863) | Style return (&pcb->dp_server); |
| 131 ↗ | (On Diff #121863) | Style return (pcb->dp_server_port); |
| 405–411 ↗ | (On Diff #121863) | Trivial but maybe we should initialize after checking the packet size since the variables won't be used in that case? |
| 434 ↗ | (On Diff #121863) | Style says unless error is boolean it should be compared to a value. != 0. |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #121912) | It'd be nice to print the error number here. |
| sys/net/debugnet.h | ||
| 216 ↗ | (On Diff #121912) | debugnet_get_server_addr() seems like a better name. |
| sys/gdb/netgdb.c | ||
|---|---|---|
| 117 | The comment still doesn't make sense. The handler should not free the mbuf chain. | |
Comment Actions
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.