From owner-svn-src-all@FreeBSD.ORG Tue Aug 9 02:16:15 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D8FFD106566C; Tue, 9 Aug 2011 02:16:15 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id 5A4F08FC08; Tue, 9 Aug 2011 02:16:14 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p792GBhv028832 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 9 Aug 2011 12:16:12 +1000 Date: Tue, 9 Aug 2011 12:16:11 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jonathan Anderson In-Reply-To: <201108082036.p78KarlR062810@svn.freebsd.org> Message-ID: <20110809105824.P896@besplex.bde.org> References: <201108082036.p78KarlR062810@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r224721 - head/sys/sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Aug 2011 02:16:15 -0000 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 , but perhaps timespecs and tv_nsec are not, since is mainly for old timeval interfaces. This is another reason why the implementation of timespec conversions belongs in where they already are and not in ). 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 starts with everything in . 2) Then there is everything in . 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 . The structure of vs is still sort of backwards. This bug became more serious when POSIX started specifying 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 is the home for them in POSIX. But POSIX doesn't specifiy all of the other pollution that is traditional or has accrufted in ). 12) Finally, there is a section that declares prototypes of user APIs. This is mostly correct (properly ifdefed). Bruce