From owner-svn-src-head@freebsd.org Thu Nov 23 15:18:57 2017 Return-Path: Delivered-To: svn-src-head@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 3A38FDEFA49; Thu, 23 Nov 2017 15:18:57 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BDBE76F4DE; Thu, 23 Nov 2017 15:18:56 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id vANFIogr035101 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 23 Nov 2017 17:18:50 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua vANFIogr035101 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id vANFIo8e035100; Thu, 23 Nov 2017 17:18:50 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 23 Nov 2017 17:18:50 +0200 From: Konstantin Belousov To: Bruce Evans Cc: 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: <20171123151849.GU2272@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> <20171123224032.A992@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171123224032.A992@besplex.bde.org> User-Agent: Mutt/1.9.1 (2017-09-22) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Nov 2017 15:18:57 -0000 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); }