Date: Fri, 24 Nov 2017 00:10:09 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326073 - head/usr.bin/systat Message-ID: <20171123224032.A992@besplex.bde.org> In-Reply-To: <20171122220538.GT2272@kib.kiev.ua> References: <201711211955.vALJtWhg047906@repo.freebsd.org> <20171122071838.R1172@besplex.bde.org> <20171122103917.GS2272@kib.kiev.ua> <20171123021646.M1933@besplex.bde.org> <20171122220538.GT2272@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 23 Nov 2017, Konstantin Belousov wrote:
> On Thu, Nov 23, 2017 at 04:24:13AM +1100, Bruce Evans wrote:
>> sysctl/sysctl.c:
>> sysctl(8) has bogus support for prettyprinting struct vmtotal (sysctl
>> shouldn't have any prettyprinting, especially not for structs that have
>> specialized programs to print them and much more). This uses intmax_t
>> for all calculations and printing except for the int16_t fields, so it
>> automatically benefited from the expansion. However since it uses a
>> correct type (signed, and not restricted to 64 bits), it now has minor
>> type errors -- it dowcasts the uint64_t to intmax_t wheen the latter is
>> 64 bits. Its Makefile uses a fairly high WARNS, so if its type errors
>> were fatal then printf format checking would have detected them (but
>> not non-fatal errors involving downcasting).
>
> Below is the cast to uintmax_t and unsigned format for sysctl(8).
This adds style bugs by expanding lines from length 79 to 81.
> diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
> index e1bf4e31914..92685a8171b 100644
> --- a/sbin/sysctl/sysctl.c
> +++ b/sbin/sysctl/sysctl.c
> @@ -625,15 +625,15 @@ S_vmtotal(size_t l2, void *p)
> "%hd Sleep: %hd)\n",
> v->t_rq, v->t_dw, v->t_pw, v->t_sl);
> printf(
> - "Virtual Memory:\t\t(Total: %jdK Active: %jdK)\n",
> - (intmax_t)v->t_vm * pageKilo, (intmax_t)v->t_avm * pageKilo);
> - printf("Real Memory:\t\t(Total: %jdK Active: %jdK)\n",
> - (intmax_t)v->t_rm * pageKilo, (intmax_t)v->t_arm * pageKilo);
> - printf("Shared Virtual Memory:\t(Total: %jdK Active: %jdK)\n",
> - (intmax_t)v->t_vmshr * pageKilo, (intmax_t)v->t_avmshr * pageKilo);
> - printf("Shared Real Memory:\t(Total: %jdK Active: %jdK)\n",
> - (intmax_t)v->t_rmshr * pageKilo, (intmax_t)v->t_armshr * pageKilo);
> - printf("Free Memory:\t%jdK", (intmax_t)v->t_free * pageKilo);
> + "Virtual Memory:\t\t(Total: %juK Active: %juK)\n",
> + (uintmax_t)v->t_vm * pageKilo, (uintmax_t)v->t_avm * pageKilo);
> + printf("Real Memory:\t\t(Total: %juK Active: %juK)\n",
> + (uintmax_t)v->t_rm * pageKilo, (uintmax_t)v->t_arm * pageKilo);
> + printf("Shared Virtual Memory:\t(Total: %juK Active: %juK)\n",
> + (uintmax_t)v->t_vmshr * pageKilo, (uintmax_t)v->t_avmshr * pageKilo);
> + printf("Shared Real Memory:\t(Total: %juK Active: %juK)\n",
> + (uintmax_t)v->t_rmshr * pageKilo, (uintmax_t)v->t_armshr * pageKilo);
> + printf("Free Memory:\t%juK", (uintmax_t)v->t_free * pageKilo);
>
> return (0);
> }
All of the casts to uintmax_t can be avoided be avoided by changing the type
of pageKilo from int to uintmax_t.
Better, do the conversion in a function-like macro pgtokb() as is done in
all (?) other utilities, but without so many overflow bugs as in most other
utiities:
#define pgtok(p) ((uintmax_t)(p) * pageKilo)
(pageKilo is back to int). This is still sloppy:
- pageKilo = getpagesize() / 1024 assumes that the page size is a multiple
of 1024 and fails very badly when the page size is 512
- the multiplication can still overflow
- when the size in K is too large to fit, the size in M or G will normally
fit and converting directly to would avoid the overflow assuming that
the page size is <= 1M.
Using floating point (even float precision) avoids all overflow and rounding
problems up to almost 128-bit sizes in bytes:
#define pgtok(p) ((p) * pageKilo) /* any integer type * float */
...
float pageKilo;
...
pageKilo = getpagesize() / 1024;
p = INT128_MAX is slightly too large to convert to float. p =
UINT64_MAX fits in float with reduced precision. No one cares about
printing more that FLT_DIG = 6 digits for memory sizes, but use double
instead of float if you want to fit INT128_MAX and print up to DBL_DIG
= 15 digits.
Then for printing, change all %jd and %ju to %.0f. In, systat, remove all
new printing functions and use putfloat.
The problem of printing large values is still there: putfloat might print
4TB bytes as 4398046511k K's or 4398047M K's (where k means 1000, K means
1024 but M means 1000000 and the K's suffix is implicit, but the new
printing function is little better -- it prints 4294962966K K's or
4194304M K's instead.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171123224032.A992>