Date: Tue, 9 Aug 2011 12:16:11 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jonathan Anderson <jonathan@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r224721 - head/sys/sys Message-ID: <20110809105824.P896@besplex.bde.org> In-Reply-To: <201108082036.p78KarlR062810@svn.freebsd.org> References: <201108082036.p78KarlR062810@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 8 Aug 2011, Jonathan Anderson wrote:
> Log:
> Create timeval2timespec() and timespec2timeval().
>
> These functions will be used by process descriptors to convert process
> creation time into process descriptor [acm]time.
These were intentionally left out.
What is wrong with the existing APIs TIMEVAL_TO_TIMESPEC() and
TIMESPEC_TO_TIMEVAL(), which are used for these conversions by almost
everything now? Well, quite a bit is wrong with them, starting with
the loudness of their names, but not including a twee spelling of "to"
in their names. The main bugs in them is that they give undocumented
APIs and namespace pollution in userland and undocumented APIs in the
kernel.
> Approved by: re (kib), mentor (rwatson)
> Suggested by: jhb
Should know better.
> Modified: head/sys/sys/time.h
> ==============================================================================
> --- head/sys/sys/time.h Mon Aug 8 19:03:26 2011 (r224720)
> +++ head/sys/sys/time.h Mon Aug 8 20:36:52 2011 (r224721)
> @@ -195,6 +195,24 @@ timeval2bintime(const struct timeval *tv
> ((tvp)->tv_usec cmp (uvp)->tv_usec) : \
> ((tvp)->tv_sec cmp (uvp)->tv_sec))
>
> +/* Conversion between timespec and timeval. */
> +
> +static __inline void
> +timeval2timespec(const struct timeval *tv, struct timespec *ts)
> +{
> +
> + ts->tv_sec = tv->tv_sec;
> + ts->tv_nsec = 1000 * tv->tv_usec;
> +}
> +
> +static __inline void
> +timespec2timeval(const struct timespec *ts, struct timeval *tv)
> +{
> +
> + tv->tv_sec = ts->tv_sec;
> + tv->tv_usec = ts->tv_nsec / 1000;
> +}
> +
> /* timevaladd and timevalsub are not inlined */
>
> #endif /* _KERNEL */
These are in the _KERNEL section, so they don't pollute userland.
Otherwise, the pollution would consist of 2 function names, 2 parameter
names and possibly 1 struct member names (I think tv_sec and tv_usec
are reserved in <sys/time.h>, but perhaps timespecs and tv_nsec are
not, since <sys/time.h> is mainly for old timeval interfaces. This
is another reason why the implementation of timespec conversions belongs
in <sys/timespec.h> where they already are and not in <sys/time.h>).
Style bugs in these include:
- use of inline functions instead of macros. Macros are used for all the
other APIs in this section. Using macros would limit the namespace
pollution. E.g., it keeps parameter names out of other namespaces.
- not using Hungrarian notation for pointers. Names of pointers are spelled
with a trailing p in all other APIs in this section.
sys/time.h has mounds of older implementation bugs. The bintime section
is especially bad. The following is mostly about buigs in the non-_KERNEL
sections.
1) Userland pollution in <sys/time.h> starts with everything in
<sys/types.h>.
2) Then there is everything in <sys/timespec.h>. The only pollution is
the undocumented TIMEPSEC conversion macros mentioned above (these
are conditional on __BSD_VISIBLE).
4) Then there is struct timezone and its members.
5) Then there is DST_* for using tz_dsttime.
6) Then there is mounds of pollution from struct bintime and its APIs.
This is conditional on __BSD_VISIBLE. Most of the bintime APIs are
undocumented. (zgrep -r bintime in /usr/share/man gives many hits,
while zgrep -r TIMESPEC in /usr/share/man gives zero hits, but most
of the hits for bintime are in peripheral man pages and bintime(9);
bintime(9) only documents the highest level of bintime APIs, leaving
all of the arithmetic and conversion APIs undocumented.)
Inlines instead of macros are used to implement most of the bintime
APIs. This gives the following undocumented pollution:
- bintime struct tag name 'bintime'
- bintime struct member names 'sec' and 'frac'. These are especially
bad since they are missing a prefix.
- all the API names
- all the parameter names: bt, x, bt2, ts, tv. Of course, these are
also missing Hungrarian notation.
- all the local variable names: u.
7) Then there is mounds of documented pollution for the NetBSD/OpenBSD
compatibility APIs. These are not under __BSD_VISIBLE, so they are
pure pollution. These were obsolete before they were born, since they
are only for timevals. The kernel has always had equivalent interfaces,
but they were intentionally left out of userland. Then they came back :-(.
But they are documented, and they implemented using macros so they are
missing the namespace pollution for parameter and local variable names,
and their parameter names are spelled in Hungrarian notations, so they
are missing most of the bugs described in this mail.
FreeBSD still doesn't have the corresponding mistakes for manipulating
timespecs. You just have to manipulate timespecs for yourself, like
you should have to do for timevals too. I prefer to convert everything
to floating point (int64_t has been usable too, ever since C99
standardized it). It is easier to multiply by 1e-6 to convert seconds
to microseconds than to remember the nonstandard APIs that manipulate
timespecs as timespecs. The timespec manipulation APIs may or may
not be faster than floating point calculations, depending on whether
the branches in them are faster than non-branchy FP code, but it is
hard to think if situations where the efficiency matters. Most uses
of the NetBSD APIs are for manipulating timeouts for things like
setitimer(2) where the syscall overcall dominates.
8) Then there is the itimer section. This is POSIX, so it is actually
permitted in this file!
9) Then there is the clockinfo section. This is pure nonstandard pollution.
It is only partially documented (in sysctl(3)).
10) Then there is the POSIX timer section (CLOCK_REALTIME... and
TIMER_ABSTIME...). This is POSIX, but is not properly ifdefed for the
versions of POSIX that support it.
11) Then there is everything in <time.h>. The structure of <sys/time.h> vs
<time.h> is still sort of backwards. This bug became more serious when
POSIX started specifying <sys/time.h> in 2001 (old versions of POSIX
didn't have timevals. Then in 2001, POSIX specified all the old timeval
APIs that it had intentionally left out in 1988, and <sys/time.h> is the
home for them in POSIX. But POSIX doesn't specifiy all of the other
pollution that is traditional or has accrufted in <sys/time.h>).
12) Finally, there is a section that declares prototypes of user APIs. This
is mostly correct (properly ifdefed).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110809105824.P896>