Date: Sat, 13 Jul 2002 05:47:18 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Alfred Perlstein <bright@mu.org> Cc: bde@freebsd.org, <audit@freebsd.org> Subject: Re: trpt cleanup Message-ID: <20020713050821.Y29226-100000@gamplex.bde.org> In-Reply-To: <20020711224521.GD97638@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 11 Jul 2002, Alfred Perlstein wrote:
> please review
>
> Index: trpt.c
> ===================================================================
> RCS file: /home/ncvs/src/usr.sbin/trpt/trpt.c,v
> retrieving revision 1.15
> diff -u -r1.15 trpt.c
> --- trpt.c 15 May 2002 09:36:46 -0000 1.15
> +++ trpt.c 11 Jul 2002 22:43:55 -0000
> @@ -82,34 +82,38 @@
> #include <paths.h>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <unistd.h>
>
> -struct nlist nl[] = {
> +struct nlist nl[3];
> #define N_TCP_DEBUG 0
> - { "_tcp_debug" },
> #define N_TCP_DEBX 1
> - { "_tcp_debx" },
> - { "" },
> -};
I agree with Thomas Quinot about this.
> ...
> +void tcp_trace(short, short, struct tcpcb *, struct tcpcb *,
> + int, void *, struct tcphdr *, int);
Style bug: continuation line misformatted in a different way (noticeably
worse) than before.
> ...
> int
> -main(argc, argv)
> - int argc;
> - char **argv;
> +main(int argc, char **argv)
> {
> int ch, i, jflag, npcbs;
> - char *system, *core;
> + const char *syst;
> + const char *core;
Style bug: 2 lines of unsorted declarations where there was only 1. I
don't like renaming variables to worse names to fix warnings about
cosmetic bugs. Unfortunately, system(3) is declared in <stdlib.h> so
it is often in (outer) scope.
> + char debug[] = "_tcp_debug";
> + char debx[] = "_tcp_debx";
> + char empty[] = "";
> +
> + bzero(nl, sizeof(nl));
> + nl[0].n_name = debug;
> + nl[1].n_name = debx;
> + nl[2].n_name = empty;
Most of this and the above nlist change seem to be to work around warnings
about string literals possibly being modified by badly designed interfaces
that don't actually modify the strings but don't say this in their prototype.
I disagree with changing the string literals to modifiable strings to break
the warning about this. The warning is really about the misdesigned
interfaces, not about the code. This problem affects dozens of programs.
Unfortunately, I can't see any good fix -- the misdesign of nlist(3) is
fundamental. All I can think of is writing the above change less verbosely:
- remove the bzero(). I think it has no effect here, but in general
bzero()ing a statically initialized struct might break the initialization
on exotic machines where NULLs or 0.0 are not all-bits-0.
- initialize nl[0].n_name to strdup("_tcp_debug") and don't use debug[], etc.
- don't initialize the last element to empty, since the last element must be
NULL according to nlist(3).
> @@ -200,7 +204,7 @@
> qsort(tcp_pcbs, npcbs, sizeof(caddr_t), numeric);
> if (jflag) {
> for (i = 0;;) {
> - printf("%x", tcp_pcbs[i]);
> + printf("%p", tcp_pcbs[i]);
Should cast the pointer to "void *". %p only has defined behaviour for
"void *"'s, and tcpcbs[i] has type caddr_t.
> @@ -314,7 +318,7 @@
> void
> tcp_trace(act, ostate, atp, tp, family, ip, th, req)
> short act, ostate;
> - struct tcpcb *atp, *tp;
> + struct tcpcb *atp __unused, *tp;
Why not just remove the unused arg?
> @@ -431,10 +435,12 @@
> printf("\n");
> if (sflag) {
> printf("\trcv_nxt %lx rcv_wnd %x snd_una %lx snd_nxt %lx snd_max %lx\n",
> - tp->rcv_nxt, tp->rcv_wnd, tp->snd_una, tp->snd_nxt,
> - tp->snd_max);
> - printf("\tsnd_wl1 %lx snd_wl2 %lx snd_wnd %x\n", tp->snd_wl1,
> - tp->snd_wl2, tp->snd_wnd);
> + (long)tp->rcv_nxt, (int)tp->rcv_wnd,
> + (long)tp->snd_una, (long)tp->snd_nxt,
> + (long)tp->snd_max);
> + printf("\tsnd_wl1 %lx snd_wl2 %lx snd_wnd %x\n",
> + (long)tp->snd_wl1,
> + (long)tp->snd_wl2, (int)tp->snd_wnd);
> }
> /* print out timers? */
> #if 0
The casts here are mostly bogus. tp->rcv_wnd and tp->snd_wnd have type
u_long, so they should be printed using %lx, not truncated so that they
can be misprinted using %x. tcp_seq has type uint32_t, so it should be
converted to u_long for printing with %lx.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020713050821.Y29226-100000>