Date: Fri, 11 Jul 2014 00:29:51 -0500 From: Bryan Venteicher <bryanv@daemoninthecloset.org> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-net@freebsd.org, hiren panchasara <hiren.panchasara@gmail.com>, John Baldwin <jhb@freebsd.org> Subject: Re: Add netbw option to systat Message-ID: <CAMo0n6RrfKagxa8none3b2RGMXDeAQCxR5GuH64pNF5xp4HtkQ@mail.gmail.com> In-Reply-To: <20140711133724.E969@besplex.bde.org> References: <CAMo0n6QD7tbObfSihfK8Sqgza-Td%2BSmfLubAcLhrBQBKfM5Fjg@mail.gmail.com> <CALCpEUEj6Nc4tVWQ0G_KF4pGTUndbBsgZnMRZWybipiQPWYpUQ@mail.gmail.com> <201407101314.58558.jhb@freebsd.org> <20140711133724.E969@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 10, 2014 at 11:50 PM, Bruce Evans <brde@optusnet.com.au> 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 >>> <bryanv@daemoninthecloset.org> 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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMo0n6RrfKagxa8none3b2RGMXDeAQCxR5GuH64pNF5xp4HtkQ>