From owner-svn-src-all@freebsd.org Thu Nov 23 13:10:18 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C8CC1DEC5D7; Thu, 23 Nov 2017 13:10:18 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 4B5496A873; Thu, 23 Nov 2017 13:10:18 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 5A12D1A2695; Fri, 24 Nov 2017 00:10:10 +1100 (AEDT) Date: Fri, 24 Nov 2017 00:10:09 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Bruce Evans , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326073 - head/usr.bin/systat In-Reply-To: <20171122220538.GT2272@kib.kiev.ua> Message-ID: <20171123224032.A992@besplex.bde.org> 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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=DczsMeIL0fzAE1lkYZ0A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Nov 2017 13:10:18 -0000 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