Fix netgdb double ack, print proxy address

[john.reimer_dell.com]

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

[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 ↗

(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.

[john.reimer_dell.com]

Style Fixes

[markj]

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.

[john.reimer_dell.com]

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.

[markj]

In D40064#913533, @john.reimer_dell.com wrote:

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 ↗

(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.

[john.reimer_dell.com]

Review comments 2

[john.reimer_dell.com]

Addressed review comments.

[markj]

sys/gdb/netgdb.c

117

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

[john.reimer_dell.com]

Fix netgdb_rx comment

[markj]

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.

[john.reimer_dell.com]

Reuploading the full set of changes