headers that use "struct bintime"

[Bruce Evans]

On Sun, 27 May 2012, Robert Millan wrote:

> 2012/5/19 Konstantin Belousov <kostikbel at gmail.com>:
>>> sys/arm/include/cpu.h
>>> sys/dev/iscsi/initiator/iscsivar.h
>>> sys/geom/journal/g_journal.h
>>> sys/sys/dtrace_bsd.h
>>> sys/sys/devicestat.h
>>> sys/sys/timeet.h
>>> sys/sys/bio.h
>>> sys/opencrypto/cryptodev.h
>>>
>> Note that all headers you listed are kernel headers, and kernel is exposed
>> to the whole namespace. I suspect that no headers are supposed to be used
>> by usermode among the list.
>
> There's at least one case (sys/devicestat.h) which is widely exposed
> to userland:

devstat has silly APIs, with long double values (despite needing the
range and precision of long doubles over doubles less than most things)
and bintimes (despite needing the precision of bintimes over less than
most things), but is well established so is hard to fix now.

> lib/libdevstat/devstat.h:#include <sys/devicestat.h>
> lib/libgeom/geom_stats.c:#include <sys/devicestat.h>
> usr.bin/kdump/ioctl.c:#include <sys/devicestat.h>

devicestat.h doesn't even have any ioctls in it, but it is apparently
needed here to supply pollution in other headers.  Perhaps it is no
longer even needed here.  In FreeBSD-4, it was needed for the include
of <sys/ccdvar.h>, which does have ioctls in it and also has a
complete struct devstat in its softc.  Headers with softcs in them
never belonged in <sys>, and softcs never belonged in public APIs,
and at least the <sys> part of this has been fixed -- <sys/ccdvar.h>
no longer exists, and the only struct devstats in <sys> are an
incomplete one in bio.h and of course the full one in devicestat.h.
ccd was from NetBSD, and has gone away completely.  FreeBSD used
mainly vn at first, then md.

I forget if you need bintimes to work when !__BSD_VISIBLE, or just
need the headers to be self-sufficient.  devicestat.h already has
the latter.  It includes sys/time.h.  The early include of sys/time.h
in kdump/ioctl.c has no effect, since it is after the include of
sys/devicestat.h where sys/time.h has already been included nested.
OTOH, in the kernel the include of sys/time.h in sys/devicestat.h
normally has no effect, since normally sys/param.h is included earlier
and it supplies sys/time.h as standard pollution.

> sbin/mdconfig/mdconfig.c:#include <sys/devicestat.h>

/dev/md's softc is ugly and includes a struct devstat, but its ugliness
is not public (it is only in dev/md/md.c).  Otherwise it would give
the same problem as ccd used to.

> and also into sys/cam/, some of which is in userland too (built into libcam):
>
> sys/cam/ata/ata_pmp.c:#include <sys/devicestat.h>
> sys/cam/ata/ata_da.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_pt.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_pass.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_targ_bh.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_sa.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_da.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_target.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_sg.c:#include <sys/devicestat.h>
> sys/cam/scsi/scsi_cd.c:#include <sys/devicestat.h>

I wonder why scsi is still doing so much with devstat.  For disks,
devstat handling mostly moved into geom.  In the old ata driver,
there are no remaining reference to struct devstat or devicestat.h
in any disk driver.  This works for ad, but IIRC device statistics
were broken for a long time for acd.  Non-disk drivers that don't
go through geom must still do their own devstat handling.  Scsi
drivers always did, and the above shows them still doing it, but
ata_da, da and cd are disk drivers so why do they do it?  Old ata
drivers mostly didn't, so device statistics never worked for them.
Now, only the ata tape driver does its own device statistics, as
it always did.

All of the above are .c files and all of the struct devstats in their
softc's are private, so they don't affect userland.

The struct devstats in the above are:

scsi/scsi_ch.c:	struct devstat	*device_stats;
scsi/scsi_pass.c:	struct devstat	*device_stats;
scsi/scsi_pt.c:	struct	 devstat *device_stats;
scsi/scsi_sa.c:	struct		devstat *device_stats;
scsi/scsi_sg.c:	struct devstat		*device_stats;
scsi/scsi_targ_bh.c:	struct		devstat device_stats;
scsi/scsi_target.c:	struct devstat		 device_stats;

The cam/ata files don't use any devstat functions, so the includes of
devicestat.h in them are apparently bogus.

cd and sa do use devstat functions, so the includes of devicestat.h
in them are needed, but they don't appear in the previous list because
they put the devstat struct in a general disk/geom struct instead of
in their softc.

scsi_ch.c includes devicestat.h and needs it but is not in your list.
This and the others not already described must do their own devstat
handling since they are not disks.  All except the targ* ones only
use an incomplete struct device_stats (a pointer to a full one).  For
disk drivers, this indirection helps avoid polluting public disk
headers with the complete declaration.  It might not be useful here,
but all the non-targ* scsi drivers do it.  In FreeBSD-4 (before geom),
_all_ scsi drivers used a complete device_stats struct in their softc.

Oops, I forgot the libcam use of kernel files.  It seems to use only
scsi_da.c and scsi_sa.c from the above.  Both of these use only an
indirect struct device_stats in their softc.  They can't reasonably
access this in userland, and it is clear that scsi_da.c tries not to
since only includes devicestat.h under a _KERNEL ifdef.  scsi_sa.c
is not so careful.  I checked this in the userland .depend.
devicestat.h is only depended on by scsi_sa.c.  Hopefully it can be
fixed in the same way as scsi_da.c was (I guess the latter does less
with devstat because more is done in geom).

Thus there seems to be no real need for devicestat.h from scsi files
userland.

The public interface for libdevstat is <devstat.h>.  This shouldn't
export struct device_stat, but it includes <sys/devicestat.h> and
struct device_stat isn't in the _KERNEL section there.  Even the
libdevstat implementation never references struct device_stat, so
it seems to be pure pollution in libdevstat.  I think libdevstat
just uses a sysctl that converts kernel struct devicestats into
userland struct devstat, so userland should never see the former.
However, bintimes are part of the public API (they are in struct
devstat and a couple of functions).

Bruce