From owner-freebsd-audit Fri Jul 12 12:44:13 2002 Delivered-To: freebsd-audit@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 31FB037B400; Fri, 12 Jul 2002 12:44:07 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id EAC0843E3B; Fri, 12 Jul 2002 12:44:05 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id FAA17537; Sat, 13 Jul 2002 05:43:54 +1000 Date: Sat, 13 Jul 2002 05:47:18 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Alfred Perlstein Cc: bde@freebsd.org, Subject: Re: trpt cleanup In-Reply-To: <20020711224521.GD97638@elvis.mu.org> Message-ID: <20020713050821.Y29226-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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 > #include > #include > +#include > #include > > -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 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