Date: Fri, 21 Jun 2019 02:43:13 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alan Somers <asomers@freebsd.org> 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: <20190621013236.N5105@besplex.bde.org> In-Reply-To: <CAOtMX2jt1M6mQNC_JekLMRfxgo2OxeR3VkfuQZHDgx%2B_vzFNWQ@mail.gmail.com> References: <201906201435.x5KEZTqH021513@repo.freebsd.org> <54f3bc97cbb485cdcc44b81c82c149ac9e46d42f.camel@freebsd.org> <CAOtMX2jt1M6mQNC_JekLMRfxgo2OxeR3VkfuQZHDgx%2B_vzFNWQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190621013236.N5105>