From owner-freebsd-net@FreeBSD.ORG Fri Jul 11 05:36:53 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 44C5CB73 for ; Fri, 11 Jul 2014 05:36:53 +0000 (UTC) Received: from mail-vc0-f181.google.com (mail-vc0-f181.google.com [209.85.220.181]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EF81D2F8F for ; Fri, 11 Jul 2014 05:36:52 +0000 (UTC) Received: by mail-vc0-f181.google.com with SMTP id hu12so1084425vcb.40 for ; Thu, 10 Jul 2014 22:36:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=PksXmLr3loTeI20ld97LyjSUVM3yaO9Zm/jidiP31LY=; b=GOJattoQXT9/hnxqd0MR+VqOiQVCXvLN0Nc6D46tbhrwZpKbiDCSr4NzahFcdpAMi5 0YjSbgLbfEl+btERvie5p3kbmyun6zv8d5+OJk/F5aeAzcaSWDjD061KggRpNtusK7Hz ktYPS4490jCCVDes4dpdt4RdalJIAEmNuXpQc2x31YXp1A/qS9vxZ7We82jDZ5mU9Cj4 hmomLlPTkPlSYecKhaH0gS31AZmsdRt7y1j2MOLnGYA7WlfVqF5r2QObLaooi9cvAAee kKC1fWCk56rjE5ONfb+6GmKCyZbjdVq6UvmHE+3zJUQ+K+/i6gb5XhhsupF8S//3y4Fb MqSA== X-Gm-Message-State: ALoCoQlOgY1W52SgEADHlMMF88owYY4n7qehwPJP8ly3OGBc+1ITLxrwKOE+2HRJogkIUHCAYBzl X-Received: by 10.220.2.136 with SMTP id 8mr5011352vcj.17.1405056611431; Thu, 10 Jul 2014 22:30:11 -0700 (PDT) MIME-Version: 1.0 Received: by 10.58.11.167 with HTTP; Thu, 10 Jul 2014 22:29:51 -0700 (PDT) X-Originating-IP: [72.177.8.109] In-Reply-To: <20140711133724.E969@besplex.bde.org> References: <201407101314.58558.jhb@freebsd.org> <20140711133724.E969@besplex.bde.org> From: Bryan Venteicher Date: Fri, 11 Jul 2014 00:29:51 -0500 Message-ID: Subject: Re: Add netbw option to systat To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.18 Cc: freebsd-net@freebsd.org, hiren panchasara , John Baldwin X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jul 2014 05:36:53 -0000 On Thu, Jul 10, 2014 at 11:50 PM, Bruce Evans wrote: > On Thu, 10 Jul 2014, John Baldwin wrote: > > On Wednesday, July 02, 2014 8:54:41 pm hiren panchasara wrote: >> >>> On Wed, Jul 2, 2014 at 4:50 PM, Bryan Venteicher >>> wrote: >>> >>>> I'd like to commit this if anybody else thinks they'd find it useful. >>>> >>>> http://people.freebsd.org/~bryanv/patches/systat-netbw.patch >>>> >>> ... >> >> 4) Should numtok() just be humanize_number? Or rather, would it simplif= y >> the code to use humanize_number? (It might not, but if it does, I >> think that would be preferable.) >> > > No, nothing should use dehumanize(scientificize)_number(). It is a > badly designed API that doesn't even support unsigned numbers or > intmax_t. But numtok() takes a floating point arg. (It is always > used for rates that really do need floating point or perhaps a > quotient of integers. Except under #if 0, it is called on intger > args. It doesn't support this case since it always geenrates a > %5.Nf format with N > 0 (except for numbers < 0.001 it prints nothing, > perhaps because %f format doesn't work well for this case). > > systat already hads the better functions putint(), putfloat() and > putlongdouble(). Unfortunately, these are static in vmstat.c. They > should be used throughout systat to get a consistent format. > numtok() takes a double arg and could be handled at some cost in > efficiency by putlongdouble(). (putlongdouble() only exists due > to design errors in libdevstat. Old parts of systat -v use floats > and floats are more than adequate, but libdevstat uses long doubles. > Probably downcasting the long doubles to float would work in > systat -v, but putfloat() was cloned to create putlongdouble() > instead.) > > putlongdouble() prints the output, but numtok() formats the output for > printing and returns it in a static buffer with MAXINDEXES =3D 8 > generations so that this method is not too fragile. In general, direct > printing is easier to use, but here the output has be printed at a > certain place in the window. putlongdouble() takes coordinates for > each call and prints using move(), addch() and addstr(). It has more > control over padding characters than printf() can provide or > humanize_number() can dream of, and uses this to padd with '*' in some > cases (mainly for numbers that cannot be formatted to fit in the desired > space. Callers must pass some format info, especially the field width, > in each call. numtok() basically hard-codes a field width of 6 and a > format of %5.N%c where %c is the suffix. This format wastes 1 character > for the suffix when the suffix is ' '. putlongdouble() and even > dehumanize_number() avoid this wastage. This is more critical when the > field with is < 6. N > 0 and the decimal point also waste a lot of > space. putlongdouble() has complications to print 100% as 100% and > not 100.0% (the latter is 2 characters wider, and especially wasteful). > dehumanize_number() doesn't have any of the complications for the > decimal point since it doesn't support floating point. > > systat -v (vmstat.c) is mostly written in KNF, but the patch has mounds > of style bugs, e.g.: > > =E2=80=8BI was trying to keep the diff small with upstream. =E2=80=8BI'll m= ake a cleanup sweep and incorporate your comments. Thanks. +void > +shownetbw(void) > +{ > + double delta_time; > + struct mytcpcb *elm, *telm; > + int row; > + > + delta_time =3D (double)(tv_curr.tv_sec - tv_last.tv_sec) - 1.0 + > + (tv_curr.tv_usec + 1000000 - tv_last.tv_usec) / 1e6; > + if (delta_time < 0.1) > + return; > + > + mvwprintw(wnd, 0, 0, > + "tcp accepts %s connects %s " > + " rcv %s snd %s rexmit %s", > + numtok(DELTARATE(tcps_accepts)), > + numtok(DELTARATE(tcps_connects) - > DELTARATE(tcps_accepts)), > + numtok(DELTARATE(tcps_rcvbyte)), > + numtok(DELTARATE(tcps_sndbyte)), > + numtok(DELTARATE(tcps_sndrexmitbyte))); > + ... > > In KNF, the continuation indent is 4. This helps minimize line splitting= , > and lines are not split unnecessarily. Fixing this and other style bugs > and pessimizations gives: > > @ struct mytcpcb *elm, *telm; > @ double delta_time; > @ int row; > @ > @ delta_time =3D tv_curr.tv_sec - tv_last.tv_sec + > @ (tv_curr.tv_usec - tv_last.tv_usec) * 1e-6; > @ if (delta_time < 0.1) > @ return; > @ mvwprintw(wnd, 0, 0, > @ "tcp accepts %s connects %s rcv %s snd %s rexmit %s"= , > @ numtok(DELTARATE(tcps_accepts)), > @ numtok(DELTARATE(tcps_connects) - DELTARATE(tcps_accepts)), > @ numtok(DELTARATE(tcps_rcvbyte)), > @ numtok(DELTARATE(tcps_sndbyte)), > @ numtok(DELTARATE(tcps_rcvbyte)), > @ numtok(DELTARATE(tcps_sndrexmitbyte))); > @ ... > > This maps fairly easily to putlongdouble(). You would have to pass windo= w > coordinates to each call. Maintaining these coordinates is fragile in > a different way than the above. The above is a combination of hard-coded > formats and field widths. It looks like it has free format except for > the initial position and the spaces before "rcv", but the fixed format > returned by numtok() turns the %s's into %.6s's modulo overflow bugs > in numtok(). > > +static const char * > +numtok(double value) > +{ > + static char buf[MAXINDEXES][32]; > + static int nexti; > + static const char *suffixes[] =3D { " ", "K", "M", "G", "T", NULL= }; > + int suffix =3D 0; > + const char *fmt; > > Initialization in declaration. Unsorted declarations. > > + > + while (value >=3D 1000.0 && suffixes[suffix+1]) { > + value /=3D 1000.0; > + ++suffix; > + } > > Missing spaces around binary operator. The initialization in the > declaration > is a good obfuscation. > > Fixing some style bugs gives: > > @ /* Next line could be compressed to single characters. */ > @ static const char * const suffixes[] =3D { > @ " ", "K", "M", "G", "T", NULL }; > @ static char buf[MAXINDEXES][32]; > @ static int nexti; > @ const char *fmt; > @ int suffix; > @ > @ suffix =3D 0; > @ while (value >=3D 1000 && suffixes[suffix + 1] !=3D NULL) { > @ value /=3D 1000; > @ suffix++; > @ } > > + nexti =3D (nexti + 1) % MAXINDEXES; > + if (value < 0.001) { > + fmt =3D " "; > + } else if (value < 1.0) { > + fmt =3D "%5.3f%s"; > + } else if (value < 10.0) { > + fmt =3D "%5.3f%s"; > + } else if (value < 100.0) { > + fmt =3D "%5.2f%s"; > + } else { > + fmt =3D "%5.1f%s"; > + } > > Excessive braces. > > + snprintf(buf[nexti], sizeof(buf[nexti]), > + fmt, value, suffixes[suffix]); > > The usual non-KNF indentation. Splitting the line isn't even needed. > The usual null error handling for snprintf() failure. Errors are > sort of handled by using a buffer of size 32 instead of 7 so that if > the value is too large for the format it is not truncated at length 6 > but messes up the format in the output. > > + return (buf[nexti]); > +} > > Bruce >