svn commit: r224721 - head/sys/sys

Alexander Best arundel at freebsd.org
Wed Aug 10 10:38:31 UTC 2011


On Tue Aug  9 11, Bruce Evans wrote:
> 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.

any reason {TIMEVAL,TIMESPEC}_TO_{TIMESPEC,TIMEVAL}()s code is being executed
in a

do { ... } while (0)

conditional loop? both macros are also defined in crypto/openssh/defines.h and
don't seem to need that extra one-time-loop.

cheers.
alex

> 
> > 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


More information about the svn-src-all mailing list