From owner-freebsd-arch@FreeBSD.ORG Wed Jan 8 06:07:28 2014 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C70FBFD0; Wed, 8 Jan 2014 06:07:28 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 5A0D011F5; Wed, 8 Jan 2014 06:07:27 +0000 (UTC) Received: from c122-106-144-87.carlnfd1.nsw.optusnet.com.au (c122-106-144-87.carlnfd1.nsw.optusnet.com.au [122.106.144.87]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id A053D1A26A9; Wed, 8 Jan 2014 17:07:17 +1100 (EST) Date: Wed, 8 Jan 2014 17:07:16 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Wemm Subject: Re: Using sys/types.h types in sys/socket.h In-Reply-To: Message-ID: <20140108154916.K969@besplex.bde.org> References: <9C1291B5-215B-440E-B8B0-6308840F755C@bsdimp.com> <20131219154839.T23018@besplex.bde.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.1 cv=bpB1Wiqi c=1 sm=1 tr=0 a=p/w0leo876FR0WNmYI1KeA==:117 a=PO7r1zJSAAAA:8 a=n4PiHyf99_4A:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=zprIhm_aaxwA:10 a=zSWcnR-bosZVrNkRHHYA:9 a=7hqjuwIJ3NRZuJKk:21 a=aV9tnsi31pFUkWv8:21 a=CjuIK1q_8ugA:10 Cc: Adrian Chadd , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jan 2014 06:07:28 -0000 On Tue, 7 Jan 2014, Peter Wemm wrote: > On Wed, Dec 18, 2013 at 9:20 PM, Bruce Evans wrote: >> On Wed, 18 Dec 2013, Adrian Chadd wrote: >> >>> Ok, how about this: >>> >>> Index: sys/sys/socket.h >>> =================================================================== >>> --- sys/sys/socket.h (revision 259475) >>> +++ sys/sys/socket.h (working copy) >>> @@ -84,6 +84,16 @@ >>> #endif >>> #endif >>> >>> +#ifndef _UINT32_T_DECLARED >>> +#define _UINT32_T_DECLARED >>> +typedef __uint32_t uint32_t; >>> +#endif >>> + >>> +#ifndef _UINTPTR_T_DECLARED >>> +#define _UINTPTR_T_DECLARED >>> +typedef __uintptr_t uintptr_t; >>> +#endif >>> + >>> /* >>> * Types >>> */ >> >> >> This seems to be correct, except the tab after the second #define is >> corrupt. Actually, all the tabs are corrupt, but the first #define >> apparently started with a tab whose corruption made a larger mess. >> >> imp@ said, in a message that should have been killfiled due to top posting, >> that this should be under __BSD_VISIBLE. That isn't strictly necessary, >> since POSIX allows names ending with _t, and it isn't very important for >> avoiding pollution since there aren't very many of them. But this is easy to do, so might as well be done. imp@ also said that the existing visibility ifdefs can be leveraged to make this especially easy to do. I missed some details of this in my reply above. Note that the ifdefs are mostly one-per-typedef, so the existing ones can't really be leveraged -- they should be replicated. The replication is to make them individually simple. >>> @@ -577,11 +587,27 @@ >>> }; >>> >>> /* >>> + * sendfile(2) kqueue information >>> + */ >>> +struct sf_hdtr_kq { >>> + int kq_fd; /* kq fd to post completion events on */ >>> + int kq_fd; /* kq fd to post completion events on */ >>> + uint32_t kq_flags; /* extra flags to pass in */ >>> + void *kq_udata; /* user data pointer */ >>> + uintptr_t kq_ident; /* ident (from userland?) */ >>> +}; > > Why can't he leave out the #ifdef/#define/typedefs and just declare it like: > int kq_fd; > __uint32_t kq_flags; > void *kq_udata; > __uintptr_t kq_ident;? > > I know it doesn't look pretty, but surely that's less painful overall than the > #ifndef _UINTPTR_T_DECLARED > #define _UINTPTR_T_DECLARED > typedef __uintptr_t uintptr_t; > #endif > thing.. We seem to do that elsewhere, eg: > struct stat { > __dev_t st_dev; /* inode's device */ > ino_t st_ino; /* inode's number */ > ... > fflags_t st_flags; /* user defined flags for file */ > __uint32_t st_gen; /* file generation number */ > __int32_t st_lspare; > ... > > What's the correct threshold for using the _DECLARED guards vs using > the __ prefixed versions? It is when either some standard requires the non-underscored types, or when using the struct members requires knowing their type (then the known type shouldn't be underscored). For the kq structs, the implementation gets to define the standard so it can do either. __dev_t in sys/stat.h is special because dev_t is only visible conditionally, but the struct members with type dev_t it must be there unconditionally. It is spelled __dev_t in the struct declararions to avoid ifdefs there. Actually, someone broke this. dev_t is now declared unconditionally, so using __dev_t in the struct declarations is just bitrot. The ifdefs would be really ugly: they would be similar to the following ones in 4.4Lite2 for timespecs: % struct stat { % dev_t st_dev; /* inode's device */ % ino_t st_ino; /* inode's number */ % mode_t st_mode; /* inode protection mode */ % nlink_t st_nlink; /* number of hard links */ % uid_t st_uid; /* user ID of the file's owner */ % gid_t st_gid; /* group ID of the file's group */ % dev_t st_rdev; /* device type */ % #ifndef _POSIX_SOURCE % struct timespec st_atimespec; /* time of last access */ % struct timespec st_mtimespec; /* time of last data modification */ % struct timespec st_ctimespec; /* time of last file status change */ % #else % time_t st_atime; /* time of last access */ % long st_atimensec; /* nsec of last access */ % time_t st_mtime; /* time of last data modification */ % long st_mtimensec; /* nsec of last data modification */ % time_t st_ctime; /* time of last file status change */ % long st_ctimensec; /* nsec of last file status change */ % #endif Here the problem is that POSIX.1-1990 (_POSIX_SOURCE ifdef) doesn't have st_*timespec fields or even timespecs. st_* is reserved, so the st_*timespec fields are allowed, but 'struct timespec' is not. Then the #define's of st_[acm]time fields in terms of the st_*timespec fields are fragile but probably standards-conforming (applications would have to do probably- undefined things to break them). I thought that FreeBSD cleaned this up by using struct __timespec instead of struct timespec in the above. Then the ifdef can be removed, and the #define's can be unifdefed too so that they handle both cases (then the treatment would be essentially the same as for __dev_t). That was one of the things struct __timespec was designed for. But this seems to have never been done (sys/stat.h included sys/_timespec.h but only used it in a secondary way). Instead, the support for POSIX.1-1990 has been broken by using struct timespec unconditionally. This is a much larger bug than defining dev_t unconditionally -- dev_t is in POSIX.1-1990; st_dev isn't there, so of course sys/stat.h isn't specified to declared dev_t, but *_t is reserved in all POSIX headers so declaring it is permitted; FreeBSD just tries not to declare *_t in all headers. struct __timespec has actually rotted into nonexistence. This turns half of the delicate layering of timespec headers into almost nonsense: - sys/_timespec.h: this existed solely to declare struct __timespec for contexts like POSIX.1-1990 where struct timespec is not permitted. It has rotted into declaring struct timespec instead. - sys/timespec.h: this existed solely to declare struct timespec for contexts where this _is_ permitted, and unfortunately to declare the bogus macros TIMEVAL_TO_TIMESPEC() and TIMESPEC_TO_TIMEVAL() (even userland depends on this undocumented pollution). Now it includes sys/_timespec.h to get the struct declaration, and it declares further pollution (struct itimerspec and its contents). Several headers are careful to include only sys/_timespec.h, so they only get struct timespec and its contents, but sys/timespec.h is included in the usual case: sys/select.h includes sys/timespec.h sys/types.h and sys/time.h include sys/select.h The pollution from this is covered by __BSD_VISIBLE, but is mostly undocumented (e.g., struct itimespec is only mentioned in a couple of POSIX timer man pages; in POSIX, it is supposed to be only in sys/time.h and in broken-as-specified headers that may have all of the symbols in sys/time.h). It is just a minor expansion of the historical pollution of sys/select.h in sys/types.h. sys/_timeval.h does the same things for timevals as sys/timespec.h does for timespecs. It exists mainly for keeping the declaration of struct timeval out of sys/types.h in the rare !__BSD_VISIBLE case. It was needed there only for the historical select.h pollution. Bruce