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

[Konstantin Belousov]

On Fri, Nov 24, 2017 at 08:15:06PM +1100, Bruce Evans wrote:
> On Thu, 23 Nov 2017, Konstantin Belousov wrote:
> 
> > On Fri, Nov 24, 2017 at 12:10:09AM +1100, Bruce Evans wrote:
> >> On Thu, 23 Nov 2017, Konstantin Belousov wrote:
> >* ...
> >>> 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
> >* ...
> >> 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.
> 
> I already pointed out the system pgtok().
> 
> It was almost correct to cast to u_int on all arches because the
> implementation can know the size of all its page counters.  That was int
> or u_int, and the expansion factor is small.  In practice, 32-bit systems
> never have enough memory to overflow in K (that happens at 4TB), and
> 64-bit systems that overflow in K are close to overflowing the page
> counters (that happens at 16TB with 4K-pages).
> 
> The pgtok() is just unusable because the page size can vary.
No, it is unusable only due to the implementation not ensuring the consistent
output type.

> 
> Perhaps it is a design error to allow the page size to vary or be
> anything except 512 in APIs, just like for disk sector sizes.  Most
> disk APIs uses units of bytes or 512-blocks.  The physical size may
> be different.  Applications need to know the physical memory page size
> even less than they need to know the physical sector size.
Not everybody share the warm memories about VAX.  I think there were no
single significant architecture with hardware support for virtual memory,
after the VAX, which used less than 4K sized pages.

> 
> Here are my old fixes for this function (to clean it up before removing it):

I picked some of this, mainly I do not want to change the output format.
I am sure that there are scripts around which parse it.

diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
index e1bf4e31914..de7fd67f258 100644
--- a/sbin/sysctl/sysctl.c
+++ b/sbin/sysctl/sysctl.c
@@ -608,33 +608,33 @@ 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);
 	}
 
-	printf(
-	    "\nSystem wide totals computed every five seconds:"
+	v = p;
+	pageKilo = getpagesize() / 1024;
+
+#define	pg2k(a)	((uintmax_t)(a) * pageKilo)
+	printf("\nSystem wide totals computed every five seconds:"
 	    " (values in kilobytes)\n");
 	printf("===============================================\n");
-	printf(
-	    "Processes:\t\t(RUNQ: %hd Disk Wait: %hd Page Wait: "
-	    "%hd Sleep: %hd)\n",
+	printf("Processes:\t\t(RUNQ: %d Disk Wait: %d Page Wait: "
+	    "%d Sleep: %d)\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));
 	return (0);
 }
 
@@ -695,8 +695,9 @@ S_efi_map(size_t l2, void *p)
 			type = types[map->md_type];
 		else
 			type = "<INVALID>";
-		printf("\n%23s %012lx %12p %08lx ", type, map->md_phys,
-		    map->md_virt, map->md_pages);
+		printf("\n%23s %012jx %12p %08jx ", type,
+		    (uintmax_t)map->md_phys, map->md_virt,
+		    (uintmax_t)map->md_pages);
 		if (map->md_attr & EFI_MD_ATTR_UC)
 			printf("UC ");
 		if (map->md_attr & EFI_MD_ATTR_WC)