svn commit: r325728 - head/lib/libkvm

[Bruce Evans]

On Mon, 4 Feb 2019, Ed Maste wrote:

> On Sat, 11 Nov 2017 at 18:31, Will Andrews <will at freebsd.org> 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 <sys/cdefs.h>

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 <sys/types.h>

Namespace pollution.  Old versions were carefully written to not have this.
They didn't use pollution like u_long, and included only <sys/_types.h>

>>  #include <nlist.h>

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 <sys/cdefs.h>
XX  #include <sys/_types.h>
XX -#include <nlist.h>
XX 
XX  /* Default version symbol. */
XX @@ -59,4 +58,5 @@
XX 
XX  struct kinfo_proc;
XX +struct nlist;
XX  struct proc;

>> +#include <vm/vm.h>

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