Re: [PATCH 2/8] additions to src/time

[Jens Gustedt <jens.gustedt@inria.fr>]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

Am Samstag, den 30.08.2014, 20:13 -0400 schrieb Rich Felker:
> On Sat, Aug 30, 2014 at 08:46:35PM +0200, Jens Gustedt wrote:
> > This adds two functions, thrd_sleep and timespec_get. Both have easy
> > functional equivalences in POSIX, but these have subtle differences in
> > the handling of errors.
> > 
> > thrd_sleep forces concrete numerical values as error return
> > 
> > timespec_get has a call interface that is incompatible with POSIX because
> > it has a bogus coding for its clock constants, and also this clock
> > constants must be returned in case of success. For the moment we only
> > implement one single clock, TIME_UTC, and map this to
> > CLOCK_REALTIME. This is the clock that we later need to measure time for
> > the timedlock and timedwait.
> > 
> > Also, other than for clock_gettime, C doesn't specify touching errno for
> > timespec_get. Because CLOCK_REALTIME can be obtained very efficiently
> > through VDSO and messing with errno is clearly the wrong step to go, we
> > try to avoid this.
> > ---
> >  src/time/thrd_sleep.c   |   26 ++++++++++++++++++++++++++
> >  src/time/timespec_get.c |   31 +++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >  create mode 100644 src/time/thrd_sleep.c
> >  create mode 100644 src/time/timespec_get.c
> > 
> > diff --git a/src/time/thrd_sleep.c b/src/time/thrd_sleep.c
> > new file mode 100644
> > index 0000000..3dbfe47
> > --- /dev/null
> > +++ b/src/time/thrd_sleep.c
> > @@ -0,0 +1,26 @@
> > +#include <time.h>
> > +#include <errno.h>
> > +#include "syscall.h"
> > +#include "libc.h"
> > +#include "threads.h"
> > +
> > +/* Roughly __syscall already returns the right thing: 0 if all went
> > +   well or a negative error indication, otherwise. */
> > +int thrd_sleep(const struct timespec *req, struct timespec *rem)
> > +{
> > +	int ret = __syscall(SYS_nanosleep, req, rem);
> > +	switch (ret) {
> > +	case 0:
> > +		return 0;
> > +		/* error described by POSIX:                                    */
> > +		/* EINTR  The nanosleep() function was interrupted by a signal. */
> > +		/* The C11 wording is:                                          */
> > +		/* -1 if it has been interrupted by a signal                    */
> 
> Purely coding style, but for musl we generally do multi-line comments
> in this form:
> 
> 	/* line 1
> 	 * line 2
> 	 * line 3 */

I don't remeber having seen that in the coding style recommendations :)

I'll fix those that I notice, and keep that in mind for future use.

> Using // would be a second-choice alternative. I don't like the
> right-alignment of */ with spaces because it will look like a mess to
> someone reading it with a non-fixed-width font.
> 
> > diff --git a/src/time/timespec_get.c b/src/time/timespec_get.c
> > new file mode 100644
> > index 0000000..20080a0
> > --- /dev/null
> > +++ b/src/time/timespec_get.c
> > @@ -0,0 +1,31 @@
> > +#include <time.h>
> > +#include "syscall.h"
> > +#include "atomic.h"
> > +
> > +static int syscall_clock_gettime(clockid_t clk, struct timespec *ts)
> > +{
> > +	return __syscall(SYS_clock_gettime, clk, ts);
> > +}
> > +
> > +void *__vdsosym(const char *, const char *);
> > +
> > +/* There is no other implemented value than TIME_UTC, all other values
> > +   are considered erroneous. */
> > +int timespec_get(struct timespec * ts, int base)
> > +{
> > +	if (base != TIME_UTC) return 0;
> > +	int ret;
> > +#ifdef VDSO_CGT_SYM
> > +	static int (*cgt)(clockid_t, struct timespec *);
> > +	if (!cgt) {
> > +		void *f = __vdsosym(VDSO_CGT_VER, VDSO_CGT_SYM);
> > +		if (!f) f = (void *)syscall_clock_gettime;
> > +		a_cas_p(&cgt, 0, f);
> > +	}
> > +	/* The vdso variants never fail, and thus never set errno. */
> > +	ret = cgt(CLOCK_REALTIME, ts);
> > +#else
> > +	ret = syscall_clock_gettime(CLOCK_REALTIME, ts);
> > +#endif
> > +	return ret < 0 ? 0 : base;
> 
> I'd rather not duplicate this code but just call __clock_gettime.

ok

> Unlike __clock_gettime, your duplicate code does not handle
> pre-clock_gettime kernels and will return an error on them.

yeah, somehow I missed the gettimeofday syscall.

Would it be acceptable by any chance to have a common core for these
two functions that doesn't set errno? It would perhaps be appropriate
to use such a function in other places of musl, too.

Jens


-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]