From owner-svn-src-all@FreeBSD.ORG Wed Aug 10 10:38:31 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 95CD41065672; Wed, 10 Aug 2011 10:38:31 +0000 (UTC) Date: Wed, 10 Aug 2011 10:38:31 +0000 From: Alexander Best To: Bruce Evans Message-ID: <20110810103831.GA60858@freebsd.org> References: <201108082036.p78KarlR062810@svn.freebsd.org> <20110809105824.P896@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110809105824.P896@besplex.bde.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Jonathan Anderson 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: Wed, 10 Aug 2011 10:38:31 -0000 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 , 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