Date: Thu, 13 Feb 2014 01:52:07 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r261796 - head/lib/libkvm Message-ID: <20140213011518.H2280@besplex.bde.org> In-Reply-To: <201402120941.s1C9fH5L011741@svn.freebsd.org> References: <201402120941.s1C9fH5L011741@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 12 Feb 2014, Gleb Smirnoff wrote: > Log: > While it isn't too late and kvm_read_zpcpu() function isn't yet used > outside libkvm(3), change its order of arguments, so that it is the > same as in kvm_read(). This also fixes some but not all namespace pollution. > Modified: head/lib/libkvm/kvm.h > ============================================================================== > --- head/lib/libkvm/kvm.h Wed Feb 12 08:04:38 2014 (r261795) > +++ head/lib/libkvm/kvm.h Wed Feb 12 09:41:17 2014 (r261796) > @@ -88,7 +88,7 @@ kvm_t *kvm_open > kvm_t *kvm_openfiles > (const char *, const char *, const char *, int, char *); > ssize_t kvm_read(kvm_t *, unsigned long, void *, size_t); > -ssize_t kvm_read_zpcpu(kvm_t *, void *, u_long, size_t, int); This shouldn't even have compiled, but someone broke kvm.h by changing its include of <sys/_types.h> to <sys/types.h>. > +ssize_t kvm_read_zpcpu(kvm_t *, unsigned long, void *, size_t, int); This fixes one dependency on the namespace pollution. > ssize_t kvm_write(kvm_t *, unsigned long, const void *, size_t); > __END_DECLS > > Modified: head/lib/libkvm/kvm_getpcpu.3 > ============================================================================== > --- head/lib/libkvm/kvm_getpcpu.3 Wed Feb 12 08:04:38 2014 (r261795) > +++ head/lib/libkvm/kvm_getpcpu.3 Wed Feb 12 09:41:17 2014 (r261796) > @@ -50,7 +50,7 @@ > .Ft void * > .Fn kvm_getpcpu "kvm_t *kd" "int cpu" > .Ft ssize_t > -.Fn kvm_read_zpcpu "kvm_t *kd" "void *buf" "u_long base" "size_t size" "int cpu" > +.Fn kvm_read_zpcpu "kvm_t *kd" "u_long base" "void *buf" "size_t size" "int cpu" This doesn't fix the documentation saying to use the namespace pollution for the changed function... > .Ft uint64_t > .Fn kvm_counter_u64_fetch "kvm_t *kd" "u_long base" ...or for other functions. This bug was missing in both the header and the man page for all of the older functions that use unsigned long (kvm_read(), kvm_uread() and kvm_write()). kvm.h otherwise depends on the full pollution of <sys/types.h> only for the declaration uint64_t. It should declare this itself like it does for all the other non-underscored typedefed types that it uses. > .Sh DESCRIPTION > > Modified: head/lib/libkvm/kvm_pcpu.c > ============================================================================== > --- head/lib/libkvm/kvm_pcpu.c Wed Feb 12 08:04:38 2014 (r261795) > +++ head/lib/libkvm/kvm_pcpu.c Wed Feb 12 09:41:17 2014 (r261796) > @@ -306,7 +306,7 @@ kvm_dpcpu_setcpu(kvm_t *kd, u_int cpu) > * Obtain a per-CPU copy for given cpu from UMA_ZONE_PCPU allocation. > */ > ssize_t > -kvm_read_zpcpu(kvm_t *kd, void *buf, u_long base, size_t size, int cpu) > +kvm_read_zpcpu(kvm_t *kd, u_long base, void *buf, size_t size, int cpu) > { > > return (kvm_read(kd, (uintptr_t)(base + sizeof(struct pcpu) * cpu), > @@ -327,7 +327,7 @@ kvm_counter_u64_fetch(kvm_t *kd, u_long > > r = 0; > for (int i = 0; i < mp_ncpus; i++) { > - if (kvm_read_zpcpu(kd, &c, base, sizeof(c), i) != sizeof(c)) > + if (kvm_read_zpcpu(kd, base, &c, sizeof(c), i) != sizeof(c)) > return (0); > r += c; > } > The implementation can reasonably use u_long after including <sys/types.h> for itself. In fact, it is a style bug to not do so. In old versions, libkvm/*.c had 1 instance of the style bug 'unsigned foo' and 105 instances of u_foo. Now it is even cleaner -- it has 0 instances of the style bug and 196 instances of u_foo. It mostly includes <sys/types.h> by including <sys/param.h>. One newer file has the style bug of including both, and another newer file is sophisticated and includes only <sys/types.h>. kvm_getfiles(3) documents a prerequesite for <sys/types.h> bogusly. This should be under the _KERNEL ifdef. And the kernel ifdef shouldn't be in the synopsis since it is not needed for calling the function but only for interpretation of the data returned by the function. It also fails to document the data format (which is an unusable mixture of struct xfile and struct file). It also fails to document the complete brokenness of this function (the function now returns with an error without actually reading any data). This is harmless because the function is never used in /usr/src. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140213011518.H2280>