Add stats(3) support to nc(1)

[trasz]

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

[0mp]

contrib/netcat/nc.1

176

s/then/them/

[allanjude]

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

[cem]

In D21324#464865, @allanjude wrote:

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

Maybe s/dump/print/ as well

contrib/netcat/netcat.c

35–39

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

45

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

133

FreeBSD_print_measurements()?

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

872–874

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.

877–882

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

1206–1207

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.

1212

Don't cast malloc in C

1218–1220

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! :)

1220–1222

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.

1228

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.

1232–1234

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

1285–1286

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]

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.

[trasz]

contrib/netcat/netcat.c

35–39

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

45

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

877–882

Hm, you're right.

1228

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

1285–1286

I'll investigate.

[trasz]

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

[trasz]

Add WITH_STATS conditional.

[trasz]

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

[cem]

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

contrib/netcat/netcat.c

985–989

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.

1228

Looks great to me, thanks

[trasz]

contrib/netcat/netcat.c

985–989

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);
}

[cem]

contrib/netcat/netcat.c

985–989

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

[allanjude]

contrib/netcat/netcat.c

1237

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

[thj]

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.

[trasz]

Remove duplicated headers, fix misguided warning, add usleep.

[trasz]

(Tinderboxed.)