Add netbw option to systat

[Bryan Venteicher]

On Thu, Jul 10, 2014 at 11:50 PM, Bruce Evans <brde at 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 at 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.:
>
>

​I was trying to keep the diff small with upstream. ​I'll make a cleanup
sweep and incorporate your comments. Thanks.


+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
>