From owner-freebsd-arch@FreeBSD.ORG Sun May 27 18:01:08 2012 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 7B2291065677; Sun, 27 May 2012 18:01:08 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id EB0708FC28; Sun, 27 May 2012 18:01:07 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q4RI0x4E019202 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 28 May 2012 04:01:00 +1000 Date: Mon, 28 May 2012 04:00:59 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Robert Millan In-Reply-To: Message-ID: <20120528023818.F2417@besplex.bde.org> References: <20120519134005.GJ2358@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Konstantin Belousov , freebsd-arch@FreeBSD.org Subject: Re: headers that use "struct bintime" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 27 May 2012 18:01:08 -0000 On Sun, 27 May 2012, Robert Millan wrote: > 2012/5/19 Konstantin Belousov : >>> 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 > lib/libgeom/geom_stats.c:#include > usr.bin/kdump/ioctl.c:#include 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 , which does have ioctls in it and also has a complete struct devstat in its softc. Headers with softcs in them never belonged in , and softcs never belonged in public APIs, and at least the part of this has been fixed -- no longer exists, and the only struct devstats in 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 /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/cam/ata/ata_da.c:#include > sys/cam/scsi/scsi_pt.c:#include > sys/cam/scsi/scsi_pass.c:#include > sys/cam/scsi/scsi_targ_bh.c:#include > sys/cam/scsi/scsi_sa.c:#include > sys/cam/scsi/scsi_da.c:#include > sys/cam/scsi/scsi_target.c:#include > sys/cam/scsi/scsi_sg.c:#include > sys/cam/scsi/scsi_cd.c:#include 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 . This shouldn't export struct device_stat, but it includes 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