Using sys/types.h types in sys/socket.h

[Bruce Evans]

On Tue, 7 Jan 2014, Peter Wemm wrote:

> On Wed, Dec 18, 2013 at 9:20 PM, Bruce Evans <brde at optusnet.com.au> 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