Date: Tue, 5 Jun 2018 15:32:00 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: Eitan Adler <eadler@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334604 - head/usr.bin/top Message-ID: <20180605141442.Y1370@besplex.bde.org> In-Reply-To: <1528128032.32688.218.camel@freebsd.org> References: <201806040527.w545R1UA075420@repo.freebsd.org> <1528128032.32688.218.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 4 Jun 2018, Ian Lepore wrote: > On Mon, 2018-06-04 at 05:27 +0000, Eitan Adler wrote: >> ... >> Log: >> \xa0 top(1): some nitpicks >> \xa0\xa0 >> \xa0 - prefer fully spelled names to "u_long" > > Why? I though we preferred the u_char, u_int, u_long spellings in BSD > code? (I certainly prefer them, and I thought style(9) did too, but I > seem to be remembering that wrong). u_foo is only prefered in system-y code (like top and the kernel). However, top used to be portable and using u_foo in it just breaks portability. However2, u_foo was only used in the MD parts of top, and was only used twice, and 1 of these uses is wrong (*). Top doesn't suffer much from unsigned poisoning, so it has little need for u_foo. It mostly uses int, and that is mostly correct. In the old MD code (just machine.c), it used 'unsigned xxx' just 3 times, and all 3 were wrong: - unsigned int swap_delay. swap_delay is boolean, but should have been int and not churned to bool. It has not been churned. - casts of len and nlen to unsigned long to print them in getsysctl(). Although the values in these variables are or should be small, the variables need to have type size_t to use them conveniently with APIs like sysctl(3). Then to print them, we bogusly cast them to unsigned long. Old versions used the wrong format %d so were broken on non-32-bit arches, and instead of fixing this properly using %zu, the warning was broken by changing the format to %lu and casting to unsigned long. unsigned long of course works unless the values are preposterous, and unsigned long is large enough for even preposterous values on all supported arches, but verifying this is more work than using %zu. Casting to int to match the old format would also work for non-preposterous values. The bogus fix was committed in 2001. I think libc supported %zu then theough this was not long after C99 standardized %zu. top/machine.c is FreeBSD-only, so it should have used the unportable %zu. (*) The use of u_long for swap_maxpages is correct. This variable is read from a kernel variable with type u_long using SYSCTL_ULONG(). The use of u_long for cpumask is now very wrong. Originally, the correct type was cpumask_t. This was u_int even on amd64. u_long was originally only slightly wrong since it is larger than u_int and cpumask is populated without asking the kernel what the CPUs are. (This method was OK. It checks for activity on all CPU numbers between 0 and ncpus - 1.) FreeBSD had fancy cpusets implemented as of arrays of (signed) long when this was new, but the kernel also used simple masks of type cpumask_t or worse, so it was limited to 32 CPUs too. Now the kernel can handle many more than 32 CPUs, but top can only handle 8 * sizeof(u_long) CPUs. top's display is also limited to 2-digit CPU numbers. style(9) says nothing about this. Its only literal match for either u_ or unsigned is in a rule saying that the C99 spelling [for fixed-width types] of uintXX_t should be used instead of the BSD spelling u_intXX_t. It hasn't suffered from unsigned poisoning and uses plain int in most examples. It doesn't even have much typedef poisoning or any examples or many rules for foo_t. Its only matches for _t are for u*_t as above, and in a rule that allows churning bool_t to bool, and in a rule that says to not use [private] typedefs ending in _t [because POSIX reserves names ending in _t]. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180605141442.Y1370>