Date: Thu, 20 Jun 2019 11:15:14 -0600 From: Alan Somers <asomers@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Ian Lepore <ian@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349233 - head/sys/sys Message-ID: <CAOtMX2h3=f_hozsrL7NqGKYafsDMzny3Z-bQh5JJ6n71Y0-9Mg@mail.gmail.com> In-Reply-To: <20190621013236.N5105@besplex.bde.org> References: <201906201435.x5KEZTqH021513@repo.freebsd.org> <54f3bc97cbb485cdcc44b81c82c149ac9e46d42f.camel@freebsd.org> <CAOtMX2jt1M6mQNC_JekLMRfxgo2OxeR3VkfuQZHDgx%2B_vzFNWQ@mail.gmail.com> <20190621013236.N5105@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jun 20, 2019 at 10:43 AM Bruce Evans <brde@optusnet.com.au> wrote: > > On Thu, 20 Jun 2019, Alan Somers wrote: > > > On Thu, Jun 20, 2019 at 9:14 AM Ian Lepore <ian@freebsd.org> wrote: > >> > >> On Thu, 2019-06-20 at 14:35 +0000, Alan Somers wrote: > >>> Author: asomers > >>> Date: Thu Jun 20 14:35:28 2019 > >>> New Revision: 349233 > >>> URL: https://svnweb.freebsd.org/changeset/base/349233 > >>> > >>> Log: > >>> #include <sys/types.h> from sys/filio.h > >>> > >>> This fixes world build after r349231 > > This increases the bugs in r349231 by adding more undocumented namespace > pollution. > > >>> Modified: head/sys/sys/filio.h > >>> ===================================================================== > >>> ========= > >>> --- head/sys/sys/filio.h Thu Jun 20 14:34:45 2019 (r349232) > >>> +++ head/sys/sys/filio.h Thu Jun 20 14:35:28 2019 (r349233) > >>> @@ -40,6 +40,7 @@ > >>> #ifndef _SYS_FILIO_H_ > >>> #define _SYS_FILIO_H_ > >>> > >>> +#include <sys/types.h> > >>> #include <sys/ioccom.h> > >>> > >>> /* Generic file-descriptor ioctl's. */ > >> > >> I wonder... is this one of those situations where it is better to use > >> __int64_t in the struct, then #include <sys/_types.h>? I think the net > >> effect there would be less pollution with other types? I've never seen > >> written guidance about when to use the __names and _types.h, but I've > >> always had the general impression that if you have to include a header > >> from another system header, it's better to use the _header.h if it > >> exists. > > > > Good question. grep shows almost equal numbers of each (37 types.h > > and 33 _types.h) in sys/sys. Do you think I should change it? > > Only low quality headers include <sys/types.h>. > > r349231 adds a struct declaration with an int64_t in it. Everything is > undocumented of course, so one knows if this header declares all the > types needed to use it. If it only declared __int64_t and used that, > then users would have to know to use __int64_t or to include a header > that declares int64_t in order to use the struct properly (e.g., > bounds checking of the __int64_t member requires knowing its type). > This is a bit inconvenient. However, since everything is undocumented, > users have to read the header to see what is in it anyway, so they can > see that it uses __int64_t just as easily as they can see the name of > the struct member of that type. > > Higher quality headers usually declare all the standard types that they > use. Some even document the types that they declare. > > filio.h is not referenced in any man page. It is normally used by > including the omnibus header <sys/ioctl.h>. <sys/ioctl.h> is documented > well enough to require it to declare all the types that it uses. This > follows from the SYNOPSIS for ioctl(2) -- since no preqrequisites for > <sys/ioctl.h> are given, it must not depend on other headers. > > ioctl(2) otherwise documents about 1% of the things needed to use it. > No one knows what macros and types <sys/ioctl.h> defines/declares. > However, the headers included by <sys/ioctl.h> used to be careful > about namespace pollution. They didn't do much more than #define lots > of macros and used only a few types and mostly only basic types. This > has rotted a bit. > > r349231 added the following bugs: > - use of int32_t, and no documentation of this > - namespace-polluting struct member names bn, runp and runb, and no > documentation of this > - comment not terminated by a ".". This is the first instance of this > bug in the file. > - space instead of tab after #define. This is the first instance of this > bug in the file. > > Previous bitrot in filio.h: > - namespace-polluting struct member names len and buf, and no documentation > of this. All struct member names in the file begin with fio, and this > would not be namespace pollution if it were documented. > - FIOSEEKDATA and FIOSEEKHOLE use the undeclared type off_t. This doesn't > break including <sys/ioctl.h>, since there is only a problem if these > macros are used. These macros are of course undocumented, so the header > must be read to even know that they exist and then it is easy to see that > off_t must be declared to use them. > > Use of uint32_t and u_long, and function parameter names in the application > namespace are not problems since they are under a _KERNEL ifdef. > > The other headers included by <sys.ioctl.h> are sys/ioccom.h, sys/sockio.h > and sys/ttycom.h: > > sys/ioccom.h: > Mostly implementation details; still fairly clean. > > sys/sockio.h: > Still defines only macros and doesn't define any of the types used by these > macros. Clearly, <sys/ioctl.h> should _not_ declare all of these types, or > even one. Defining all of them might require including all socket and > network headers. Since there is no documentation, users must read this > header to see which type they need, then read the header that declares > that type to see what its prerequisites are... > > sys/ttycom.h: > Like sys/sockio.h, but a hundred times simpler. The only undeclared types > that it uses are struct timeval and struct termios. > > Summary: <sys/ioctl.h> and the headers that it includes should declare > minimal types to compile (so __int64_t is enough). Most uses of this > header require including domain-specific headers which declare the > relevant data structures. > > Bruce Bruce, would you be satisfied by switching from <sys/types.h> to <sys/_types.h> and from int64_t to __int64_t? -Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2h3=f_hozsrL7NqGKYafsDMzny3Z-bQh5JJ6n71Y0-9Mg>