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-head
mailing list