Page MenuHomeFreeBSD

Add stats(3) support to nc(1)
ClosedPublic

Authored by trasz on Aug 19 2019, 4:56 PM.

Details

Summary

Add -M option to nc(1), which makes it return TCP connection
statistics obtained with stats(3) to standard error.

Sponsored by: Klara, Inc

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

0mp added inline comments.
contrib/netcat/nc.1
176 ↗(On Diff #61001)

s/then/them/

We should make the -M flag be compile time dependant in WITH_STATS

We should make the -M flag be compile time dependant in WITH_STATS

IMO the flag should be present either way, but WITHOUT_STATS it should errx().

contrib/netcat/nc.1
176 ↗(On Diff #61001)

Maybe s/dump/print/ as well

contrib/netcat/netcat.c
35–38 ↗(On Diff #61001)

Picking some random lines; I think the header ordering was right before? style(9):

Kernel include files (sys/*.h) come first.
... The
remaining kernel headers should be sorted alphabetically.
...

Leave a blank line before the next group, the /usr/include files, which
should be sorted alphabetically by name.

#include <stdio.h>

I'm guessing that you made this change because sys/types.h cowardly refuses to define the same types in userspace as it does in the kernel, and thus you need at least stdbool and probably stdint, if not others, before the sys/qmath or sys/stats or sys/arb includes?

I might fix that by just having those sys/ headers recursively include their dependencies in userspace builds only :-/. Or probably more contentiously, sys/types.h could be modified to include the userspace headers for the types it normally defines for the kernel.

You could also just live with a mostly style-correct and unchanged ordering, but add the handful of needed userspace headers above the sys/ ones (probably not all of these are needed).

58 ↗(On Diff #61001)

Doesn't this include need to be conditional on WITH_STATS? Without it, the file may not be installed?

132 ↗(On Diff #61001)

FreeBSD_print_measurements()?

(Is there a real active upstream for this program? Edit: I guess we regularly import updates from OpenBSD, ok.)

871–873 ↗(On Diff #61001)

There's another close() case here, although I agree it doesn't make sense to print stats in this instance (unexpected poll error). It doesn't make sense to close() immediately before err() exit, either, but I guess we inherited it from OpenBSD.

876–878 ↗(On Diff #61001)

Shouldn't we close and print measurements in this case?

1202–1203 ↗(On Diff #61001)

Maybe this is just me, but sb is commonly used for struct sbuf pointers, so the name here makes reading the following code a little confusing to me. You don't have to make any change if you don't want to, but I'd probably use a distinct name for the statsblob, like stats or statsb.

1208 ↗(On Diff #61001)

Don't cast malloc in C

1214–1216 ↗(On Diff #61001)

This kind of retry classically has the problem of never catching up to a growing structure. Although I guess if the socket is mostly shutdown maybe we can reasonably expect the true size to be static. Nevermind! :)

1216–1218 ↗(On Diff #61001)

I think you intended to initialize error rather than errno; the former makes the logic more correct (it's not now) and the latter is already set by realloc() on failure.

But I'd suggest just doing err(1, "realloc"); here instead; discussed below.

1224 ↗(On Diff #61001)

This is not a great err message; it probably makes sense to at least specify TCP and stats. And it's totally misleading if realloc failed.

1228–1230 ↗(On Diff #61001)

This function doesn't set errno on failure, it just returns a value. So you want errc(1, error, "stats_blob_tostr");

1279–1280 ↗(On Diff #61001)

Is the same proto:key really overloaded for both enabling statistics gathering and dumping the output blob? Can programs actually tell if TCP_STATS is enabled or does getsockopt(TCP, TCP_STATS) try to dump a stats blob into an int?

trasz marked 10 inline comments as done.

Fix most of the problems pointed out by cem@. There's still a problem
of building it conditional on WITH_STATS; there's some tinderboxing
to do.

contrib/netcat/netcat.c
35–38 ↗(On Diff #61001)

I think I'd prefer the third one, seems least intrusive, even if somewhat ugly.

58 ↗(On Diff #61001)

The header will be installed, it comes from sys/sys/, not from lib/libstats/.

876–878 ↗(On Diff #61001)

Hm, you're right.

1224 ↗(On Diff #61001)

Not sure what's the best way to fix it. What do you think about this version?

1279–1280 ↗(On Diff #61001)

I'll investigate.

Make -M also work with -l; reported by thj@.

Add WITH_STATS conditional.

Attempt to fix problems with -N reported by thj@; also build fix
for rescue(8).

I didn't look too closely but this look a lot better, thanks. One question below.

contrib/netcat/netcat.c
1224 ↗(On Diff #61001)

Looks great to me, thanks

993–997 ↗(On Diff #63637)

What's this one for? shutdown isn't close and we could still be reading from the remote TCP socket for some time, I think.

This revision is now accepted and ready to land.Oct 24 2019, 4:57 PM
contrib/netcat/netcat.c
993–997 ↗(On Diff #63637)

It seems you can't call getsockopt(3) on a connection that has been shut down; in this particular case it trips this check in sys/netinet/tcp_usrreq.c:tcp_ctloutput():

if (inp->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
        INP_WUNLOCK(inp);
        return (ECONNRESET);
}
contrib/netcat/netcat.c
993–997 ↗(On Diff #63637)

Huh. That seems like a kernel bug to me, although obviously extremely tangential to this review. No objection to this for now.

contrib/netcat/netcat.c
1250 ↗(On Diff #63637)

This can give the wrong error message sometimes:

nc: getsockopt(TCP_STATS) failed; kernel built without "options STATS"?
nc: getsockopt: Connection reset by peer

We should not emit the "kernel built without STATS" in this common case, it is misleading

I get inconsistent results for TXPB, you can see this with the test script here:
https://gist.github.com/adventureloop/fb68d805696aeaf03c227f08cbe87947

Allan suggested adding a 100ms usleep to the start of FreeBSD_stats_print which
does resolve this.

Remove duplicated headers, fix misguided warning, add usleep.

This revision now requires review to proceed.Dec 2 2019, 10:10 PM
trasz marked an inline comment as done.Dec 3 2019, 3:49 PM
This revision is now accepted and ready to land.Dec 4 2019, 3:05 PM