svn commit: r326073 - head/usr.bin/systat

[Konstantin Belousov]

On Fri, Nov 24, 2017 at 12:10:09AM +1100, Bruce Evans wrote:
> 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)
Amusingly there is already MD macro in machine/param.h with the same name
and same intent, but as you formulate it, sloppy implementation.  It uses
unsigned long cast on almost all 64bit arches, except powerpc.  For 32bit
arches, the cast is not done, unfortunately.

> 
> (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:
No, I do not want to use floating point calculation there.

diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
index e1bf4e31914..36851f302a0 100644
--- a/sbin/sysctl/sysctl.c
+++ b/sbin/sysctl/sysctl.c
@@ -608,14 +608,18 @@ S_timeval(size_t l2, void *p)
 static int
 S_vmtotal(size_t l2, void *p)
 {
-	struct vmtotal *v = (struct vmtotal *)p;
-	int pageKilo = getpagesize() / 1024;
+	struct vmtotal *v;
+	int pageKilo;
 
 	if (l2 != sizeof(*v)) {
 		warnx("S_vmtotal %zu != %zu", l2, sizeof(*v));
 		return (1);
 	}
 
+	v = p;
+	pageKilo = getpagesize() / 1024;
+
+#define	pg2k(a)	((uintmax_t)(a) * pageKilo)
 	printf(
 	    "\nSystem wide totals computed every five seconds:"
 	    " (values in kilobytes)\n");
@@ -624,16 +628,16 @@ S_vmtotal(size_t l2, void *p)
 	    "Processes:\t\t(RUNQ: %hd Disk Wait: %hd Page Wait: "
 	    "%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);
+	printf("Virtual Memory:\t\t(Total: %juK Active: %juK)\n",
+	    pg2k(v->t_vm), pg2k(v->t_avm));
+	printf("Real Memory:\t\t(Total: %juK Active: %juK)\n",
+	    pg2k(v->t_rm), pg2k(v->t_arm));
+	printf("Shared Virtual Memory:\t(Total: %juK Active: %juK)\n",
+	    pg2k(v->t_vmshr), pg2k(v->t_avmshr));
+	printf("Shared Real Memory:\t(Total: %juK Active: %juK)\n",
+	    pg2k(v->t_rmshr), pg2k(v->t_armshr));
+	printf("Free Memory:\t%juK", pg2k(v->t_free));
+#undef pg2k
 
 	return (0);
 }