From owner-svn-src-all@freebsd.org Tue Feb 5 10:17:49 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D730F14AF364; Tue, 5 Feb 2019 10:17:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 2DBBD6A548; Tue, 5 Feb 2019 10:17:47 +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 mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 852273D60AA; Tue, 5 Feb 2019 21:17:44 +1100 (AEDT) Date: Tue, 5 Feb 2019 21:17:43 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ed Maste cc: Will Andrews , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r325728 - head/lib/libkvm In-Reply-To: Message-ID: <20190205202145.A1080@besplex.bde.org> References: <201711112330.vABNUwXC077395@repo.freebsd.org> 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=P6RKvmIu c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=DxQ8RM24wQtPGcW6wzQA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: 2DBBD6A548 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.98)[-0.984,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Tue, 05 Feb 2019 10:17:49 -0000 On Mon, 4 Feb 2019, Ed Maste wrote: > On Sat, 11 Nov 2017 at 18:31, Will Andrews wrote: >> >> Author: will >> Date: Sat Nov 11 23:30:58 2017 >> New Revision: 325728 >> URL: https://svnweb.freebsd.org/changeset/base/325728 >> >> Log: >> libkvm: add kvm_walk_pages API. >> >> Modified: head/lib/libkvm/kvm.h >> ============================================================================== >> --- head/lib/libkvm/kvm.h Sat Nov 11 22:50:14 2017 (r325727) >> +++ head/lib/libkvm/kvm.h Sat Nov 11 23:30:58 2017 (r325728) >> @@ -36,6 +36,7 @@ >> #include Redundant. sys/types.h already includes this (more than once via nesting), and it is OK to depend on this, especially since sys/types.h depends on it much more than this header. >> #include Namespace pollution. Old versions were carefully written to not have this. They didn't use pollution like u_long, and included only >> #include Namespace pollution. Old versions have this. I fixed this in my version 15-20 years ago: XX Index: kvm.h XX =================================================================== XX RCS file: /home/ncvs/src/lib/libkvm/kvm.h,v XX retrieving revision 1.16 XX diff -u -2 -r1.16 kvm.h XX --- kvm.h 13 Oct 2003 04:44:55 -0000 1.16 XX +++ kvm.h 13 Oct 2003 04:46:29 -0000 XX @@ -40,5 +40,4 @@ XX #include XX #include XX -#include XX XX /* Default version symbol. */ XX @@ -59,4 +58,5 @@ XX XX struct kinfo_proc; XX +struct nlist; XX struct proc; >> +#include Larger, newer namespace pollution. >> >> /* Default version symbol. */ >> #define VRS_SYM "_version" >> @@ -73,7 +74,19 @@ struct kvm_swap { >> u_int ksw_reserved2; >> }; >> >> +struct kvm_page { >> + unsigned int version; This would be a style bug if the namespace pollution is allowed. 'unsigned int' is spelled u_int in the kernel and in system headers and files. But portable ones can't use it. The struct member name is also namespace pollution and a style bug. 'version' is in the application namespace and is especially likely to be used in applications, perhaps to #define it. Old structs in this header are careful to use a prefix for names. Unfortunately, the prefix in the one struct was ksw which is not obviousy related to kvm. Even the prefix kvm_ is not documented as being reserved in kvm(3). There was already massive breakage in this area: - old versions have the struct tags kinfo_proc and proc, and much more in the nlist include, but the above fix reduces this to a another struct tag. - the previous version has a private declaration of vm_prot_t to avoid the pollution of including vm/vm.h. This is nonsense now. - VRS_SYM and VRS_KEY are in the application namespace, and their names don't even give a hint that they are for kvm - member names n_* in struct kvm_nlist gives some of the pollution fixed by not including nlist.h - the type of the struct members in struct kvm_swap rotted from int to u_int - u_int for the name of type of the struct members in struct kvm_swap is pollution that is still carefully avoided in old parts of the file - the struct member names in struct kvm_page are in the apolication namespace - the struct member declarations in struct kvm_page are misformatted. They mostly use the pollution u_long, but one uses the verbose "unsigned int". The shorted type names can be formatted better - SWIF_DEV_PREFIX and LIBKVM_WALK_PAGES_VERSION are in the application namespace. The latter at least gives a hint that it is for vm - the prototype for kvm_counter_u64_fetch() uses u_long. Old prototypes are more careful - the declarations for kvm_walk_pages* are misformatted and unsorted. >> + u_long paddr; Further pollution in the type and struct member names. Further misformatting The include of sys/_types.h was apparently changed to sys/types.h to support using u_long. > This should probably be uin64_t to support cross-debugging cores from > 64-bit machines on 32-bit hosts; also for i386 PAE. Or, on IRC jhb > suggested we introduce a kpaddr_t typedef akin to kvaddr_t. The correct type is vm_paddr_t or maybe a kvm wrapper of this. kib just changed vm_paddr_t on i386. I don't like this (it gives another pessimization of i386) and it depends on a kernel option in my version. Applications can't use this kernel option so would have to use their own larger type. Even uint64_t might be too small. Hard-coding it is worse than hard-coding unsigned long. > >> + u_long kmap_vaddr; >> + u_long dmap_vaddr; > > These two should be kvaddr_t. Further pollution and style bugs in names, types and formatting. The implementation of this is another bug. It is declared in sys/types.h, so application headers like kvm.h can't use it without including lots of pollution. Maybe getting it was the original reason for changing the included and the pollution and style bugs from using u_long is bitrot. It is currently hard-coded as __uint64_t. That works for all supported arches now, but eventually some typedefs will actually be needed for their purpose of being implementation-depended and changeable. > >> + vm_prot_t prot; >> + u_long offset; > > off_t? Further pollution and style bugs in names, types and formatting. Maybe uoff_t. off_t is 64-bits signed so can't reach most kernel addresses on some 64-bit arches like amd64. off_t has the same expansion problems of any typedef'ed type. > >> + size_t len; Further pollution and style bugs 1 name and formatting. >> + /* end of version 1 */ >> +}; >> + >> #define SWIF_DEV_PREFIX 0x0002 >> +#define LIBKVM_WALK_PAGES_VERSION 1 Further pollution bugs in 1 name. The formatting is actually correct, but is inconsistent with the misformatting in the previous line. The new namespace pollution from the vm include doesn't seem to be actually used. vm_prot_t is used, but it is the one type in vm.h that is already declared here. The older change of including sys/types.h also defeats the careful ifdefs for the 2 standard types declared here. Bruce