Date: Fri, 11 Jul 2014 14:50:54 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-net@freebsd.org, Bryan Venteicher <bryanv@daemoninthecloset.org>, hiren panchasara <hiren.panchasara@gmail.com> Subject: Re: Add netbw option to systat Message-ID: <20140711133724.E969@besplex.bde.org> In-Reply-To: <201407101314.58558.jhb@freebsd.org> References: <CAMo0n6QD7tbObfSihfK8Sqgza-Td%2BSmfLubAcLhrBQBKfM5Fjg@mail.gmail.com> <CALCpEUEj6Nc4tVWQ0G_KF4pGTUndbBsgZnMRZWybipiQPWYpUQ@mail.gmail.com> <201407101314.58558.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 simplify > 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 = 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.: +void +shownetbw(void) +{ + double delta_time; + struct mytcpcb *elm, *telm; + int row; + + delta_time = (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 = 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 window 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[] = { " ", "K", "M", "G", "T", NULL }; + int suffix = 0; + const char *fmt; Initialization in declaration. Unsorted declarations. + + while (value >= 1000.0 && suffixes[suffix+1]) { + value /= 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[] = { @ " ", "K", "M", "G", "T", NULL }; @ static char buf[MAXINDEXES][32]; @ static int nexti; @ const char *fmt; @ int suffix; @ @ suffix = 0; @ while (value >= 1000 && suffixes[suffix + 1] != NULL) { @ value /= 1000; @ suffix++; @ } + nexti = (nexti + 1) % MAXINDEXES; + if (value < 0.001) { + fmt = " "; + } else if (value < 1.0) { + fmt = "%5.3f%s"; + } else if (value < 10.0) { + fmt = "%5.3f%s"; + } else if (value < 100.0) { + fmt = "%5.2f%s"; + } else { + fmt = "%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?20140711133724.E969>