Date: Wed, 9 Dec 2015 18:19:16 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Hajimu UMEMOTO <ume@freebsd.org> Cc: Eric van Gyzen <vangyzen@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r291994 - head/include Message-ID: <20151209173008.A828@besplex.bde.org> In-Reply-To: <yge1tawwei9.wl-ume@FreeBSD.org> References: <201512081609.tB8G9mfd053070@repo.freebsd.org> <yge1tawwei9.wl-ume@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 9 Dec 2015, Hajimu UMEMOTO wrote:
> Hi,
>
>>>>>> On Tue, 8 Dec 2015 16:09:48 +0000 (UTC)
>>>>>> Eric van Gyzen <vangyzen@FreeBSD.org> said:
> ...
> vangyzen> Log:
> vangyzen> resolver: fix the build of some ports, broken by r289315
> vangyzen>
> vangyzen> r289315 required time_t and struct timespec to be defined before
> vangyzen> including <resolv.h>. This broke the build of net-mgmt/sx, at least.
> vangyzen>
> vangyzen> Include <sys/timespec.h> in resolv.h to fix this with minimal pollution.
> ...
> It seems wrong. We should not have pollution from timespec.h, here.
> You should consider moving them into __res_state_ext like NetBSD does.
resolv.h already had massinve namespace pollution and style bugs in
its includes. One more include of a header that is relatively clean
since it is tiny and was designed for minimising namespace pollution
makes little difference.
Old namespace pollution and style bugs:
X #include <sys/param.h>
This includes the kitchen sink via nested includes (not as many as in the
kernel). Almost none of its namespace is documented. Even the names that
it is supposed to define are mostly undocumented, so no one knows what these
are.
X #include <sys/types.h>
This was already included recursively at least ones. style(9) explicitly
forbids including this again.
X #include <sys/cdefs.h>
This was already included recursively approximitately <number of polluting
other nested includes> times. Include guards actually prevent it being
parsed more than the first time. This is the only included header that is
supposed to have no namespace pollution. style(9) has less to say about
this. It makes more sense to include it first, and that is the correct
style (for __FBSDID) in .c files. Headers with no pollution usually need
to include it directly. Others should depend on primary headers like
<sys/types.h> including it.
X #include <sys/socket.h>
X #include <stdio.h>
X #include <arpa/nameser.h>
Further pollution.
sys/timespec.h adds to this just:
Y #include <sys/cdefs.h>
Example of a correct use of sys/cdefs.h. It is to get the definition
of __BSD_VISIBLE. However, use of that is bogus.
Y #include <sys/_timespec.h>
sys/_timespec was originally correctly designed have no namespace
pollution. It declared a struct __timespec and use __time_t in it,
since timespec and time_t would be namespace pollution in some cases.
It should be included in places like <sys/stat.h> where __timespec
etc. would be pollution.
sys/timespec.h was originally correctly designed turn __timespec into
timespec and __time_t into timespec (except it also defines the bogus
conversion macros -- see below). It should be included in the very
few places where __timespec would not be pollution in any supported
version, and for traditional pollution in places like time.h.
Using these little headers is delicate and usually does nothing except
increase compilation times by including both of them nested. Having
2 for just struct timespec was a bit much, and has been turned into
nonsense. Someone changed __timespec to timespec and __time_t to
time.h in _timespec.h, and removed the careful anti-pollution ifdefs
in <sys/stat.h>, with the justification that timespec is now standard
in POSIX. But it isn't standard in old versions of POSIX. So this
change mainly broke <sys/stat.h> and turned _timespec.h's existence
into nonsense. It didn't break much else since not much else was
careful about this. POSIX now requires timespec to be declared in
many headers and allows but doesn't require it to be declared (as
pollution) in many others.
Y
Y #if __BSD_VISIBLE
Y #define TIMEVAL_TO_TIMESPEC(tv, ts) \
Y do { \
Y (ts)->tv_sec = (tv)->tv_sec; \
Y (ts)->tv_nsec = (tv)->tv_usec * 1000; \
Y } while (0)
Y #define TIMESPEC_TO_TIMEVAL(tv, ts) \
Y do { \
Y (tv)->tv_sec = (ts)->tv_sec; \
Y (tv)->tv_usec = (ts)->tv_nsec / 1000; \
Y } while (0)
Y
Y #endif /* __BSD_VISIBLE */
These macros are bogus. They are bad enough in the kernel. Of course
they are not documented in any man page. sys/timespec.h was original
clean except for defining these.
Y
Y /*
Y * Structure defined by POSIX.1b to be like a itimerval, but with
Y * timespecs. Used in the timer_*() system calls.
Y */
Y struct itimerspec {
Y struct timespec it_interval;
Y struct timespec it_value;
Y };
This pollution was added later. It turns timespec.h into half-nonsense.
timespec.h was supposed to match its name and declare only struct timespec.
The pollution here implements the POSIX requirement that struct itimerspec
is declared in <time.h>.
<time.h> and <sys/time.h> have much worse namespace and organization
problems than resolv.h. There is also sys/_timeval.h which does for
timevals what sys/timespec.h should do for timespecs. It is relatively
simple and clean. In fact, it has always been almost as perfect as
possible except for its name -- it is unnecessary and confusing for
it to be named with an underscore, especially when the corresponding
file for timespecs is not named with an underscore.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151209173008.A828>