[LTP] [PATCH V2 2/2] syscalls/clock_gettime: Add support for time64 tests

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +/*
> + * timespec_updated routines return:
> + * 0: On success, i.e. timespec updated correctly.
> + * -1: Error, timespec not updated.
> + * -2: Error, tv_nsec is corrupted.
> + */
> +static inline int tst_timespec_updated_32(void *data)
> +{
> +	struct timespec *spec = data;
> +
> +	return (spec->tv_nsec != 0 || spec->tv_sec != 0) ? 0 : -1;
> +}

Shouldn't this be called tst_timespec_updated() instead to keep the
function names consistent. Also it would be a good idea to name it
tst_timespec_nonzero() which describes the function better.

> +static inline int tst_timespec_updated_64(void *data)
> +{
> +	struct __kernel_timespec *spec = data;
> +
> +	if (spec->tv_nsec != 0 || spec->tv_sec != 0) {
> +		/* Upper 32 bits of tv_nsec should be cleared */
> +		if (spec->tv_nsec >> 32)
> +			return -2;
> +		else
> +			return 0;
> +	} else {
> +		return -1;
> +	}
> +}


And this should be tst_timespec64_nonzero().

>  /*
>   * Converts timeval to microseconds.
>   */
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime.h b/testcases/kernel/syscalls/clock_gettime/clock_gettime.h
> new file mode 100644
> index 000000000000..6976e6884de9
> --- /dev/null
> +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime.h
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#ifndef CLOCK_GETTIME_H
> +#define CLOCK_GETTIME_H
> +
> +#include "tst_timer.h"
> +#include "tst_test.h"
> +#include "lapi/syscalls.h"
> +
> +#ifdef TST_ABI32
> +static inline int libc_clock_gettime(clockid_t clk_id, void *tp)
> +{
> +	return clock_gettime(clk_id, tp);
> +}
> +#endif
> +
> +static inline int sys_clock_gettime(clockid_t clk_id, void *tp)
> +{
> +	return tst_syscall(__NR_clock_gettime, clk_id, tp);
> +}
> +
> +#if (__NR_clock_gettime64 != __LTP__NR_INVALID_SYSCALL)
> +static inline int sys_clock_gettime64(clockid_t clk_id, void *tp)
> +{
> +	return tst_syscall(__NR_clock_gettime64, clk_id, tp);
> +}
> +#endif

I guess that we can drop these ifdefs, because the defitions would
compile fine regardless and will not produce warnings if unused.

> +#endif /* CLOCK_GETTIME_H */
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime01.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime01.c
> index d365823b2f0f..001ac3049a23 100644
> --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime01.c
> +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime01.c
> @@ -17,22 +17,17 @@
>   */
>  
>  #include "config.h"
> -#include "tst_timer.h"
>  #include "tst_safe_clocks.h"
> -#include "tst_test.h"
> -#include "lapi/syscalls.h"
> +#include "lapi/abisize.h"
> +
> +#include "clock_gettime.h"
>  
>  struct test_case {
>  	clockid_t clktype;
>  	int allow_inval;
>  };
>  
> -struct tmpfunc {
> -	int (*func)(clockid_t clk_id, struct timespec *tp);
> -	char *desc;
> -};
> -
> -struct test_case tc[] = {
> +static struct test_case tc[] = {
>  	{
>  	 .clktype = CLOCK_REALTIME,
>  	 },
> @@ -63,73 +58,71 @@ struct test_case tc[] = {
>  	 },
>  };
>  
> -static int sys_clock_gettime(clockid_t clk_id, struct timespec *tp)
> -{
> -	return tst_syscall(__NR_clock_gettime, clk_id, tp);
> -}
> +#ifdef TST_ABI32
> +static struct timespec spec32;

Shouldn't we just call this one spec? It's a glibc type which may switch
to 64bit in the future, I wouldn't assume anything here.

Maybe we should decouple the __kernel_old_timespec and struct timespec
completely so that the check functions are separate as well.

Does anyone know what is the plan for glibc?

> +static struct __kernel_old_timespec kspec32;
> +#endif
> +
> +static struct __kernel_timespec kspec64;
> +
> +static struct test_variants {
> +	int (*func)(clockid_t clk_id, void *tp);
> +	int (*check)(void *spec);
> +	void *spec;
> +	int spec_size;
> +	char *desc;
> +} variants[] = {
> +#if defined(TST_ABI32)
> +	{ .func = libc_clock_gettime, .check = tst_timespec_updated_32, .spec = &spec32, .spec_size = sizeof(spec32), .desc = "ABI32 vDSO or syscall"},
> +	{ .func = sys_clock_gettime, .check = tst_timespec_updated_32, .spec = &spec32, .spec_size = sizeof(spec32), .desc = "ABI32 syscall with libc spec"},
> +	{ .func = sys_clock_gettime, .check = tst_timespec_updated_32, .spec = &kspec32, .spec_size = sizeof(kspec32), .desc = "ABI32 syscall with kernel spec"},
> +#endif
> +
> +#if defined(TST_ABI64)
> +	{ .func = sys_clock_gettime, .check = tst_timespec_updated_64, .spec = &kspec64, .spec_size = sizeof(kspec64), .desc = "ABI64 syscall with kernel spec"},
> +#endif
> +
> +#if (__NR_clock_gettime64 != __LTP__NR_INVALID_SYSCALL)
> +	{ .func = sys_clock_gettime64, .check = tst_timespec_updated_64, .spec = &kspec64, .spec_size = sizeof(kspec64), .desc = "ABI64 syscall time64 with kernel spec"},
> +#endif
> +};
>  
> -static int check_spec(struct timespec *spec)
> +static void setup(void)
>  {
> -	return (spec->tv_nsec != 0 || spec->tv_sec != 0) ? 1 : 0;
> +	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
>  }
>  
>  static void verify_clock_gettime(unsigned int i)
>  {
> -	size_t sz;
> -	struct timespec spec;
> +	struct test_variants *tv = &variants[tst_variant];
> +	int ret;
>  
> -	/*
> -	 * check clock_gettime() syscall AND libc (or vDSO) functions
> -	 */
> -	struct tmpfunc tf[] = {
> -		{ .func = sys_clock_gettime, .desc = "syscall"      },
> -		{ .func = clock_gettime, .desc = "vDSO or syscall"  },
> -	};
> +	memset(tv->spec, 0, tv->spec_size);
>  
> -	for (sz = 0; sz < ARRAY_SIZE(tf); sz++) {
> -
> -		memset(&spec, 0, sizeof(struct timespec));
> -
> -		TEST(tf[sz].func(tc[i].clktype, &spec));
> -
> -		if (TST_RET == -1) {
> -
> -			/* errors: allow unsupported clock types */
> -
> -			if (tc[i].allow_inval && TST_ERR == EINVAL) {
> -
> -				tst_res(TPASS, "clock_gettime(2): unsupported "
> -						"clock %s (%s) failed as "
> -						"expected",
> -						tst_clock_name(tc[i].clktype),
> -						tf[sz].desc);
> -
> -			} else {
> -
> -				tst_res(TFAIL | TTERRNO, "clock_gettime(2): "
> -						"clock %s (%s) failed "
> -						"unexpectedly",
> -						tst_clock_name(tc[i].clktype),
> -						tf[sz].desc);
> -			}
> +	TEST(tv->func(tc[i].clktype, tv->spec));
>  
> +	if (TST_RET == -1) {
> +		/* errors: allow unsupported clock types */
> +		if (tc[i].allow_inval && TST_ERR == EINVAL) {
> +			tst_res(TPASS, "clock_gettime(2): unsupported clock %s failed as expected",
> +				tst_clock_name(tc[i].clktype));
>  		} else {
> +			tst_res(TFAIL | TTERRNO, "clock_gettime(2): clock %s failed unexpectedly",
> +				tst_clock_name(tc[i].clktype));
> +		}
>  
> -			/* success: also check if timespec was changed */
> -
> -			if (check_spec(&spec)) {
> -				tst_res(TPASS, "clock_gettime(2): clock %s "
> -						"(%s) passed",
> -						tst_clock_name(tc[i].clktype),
> -						tf[sz].desc);
> -			} else {
> -
> -				tst_res(TFAIL, "clock_gettime(2): clock %s "
> -						"(%s) passed, unchanged "
> -						"timespec",
> -						tst_clock_name(tc[i].clktype),
> -						tf[sz].desc);
> -			}
> +	} else {
> +		/* success: also check if timespec was changed */
> +		ret = tv->check(tv->spec);
> +		if (!ret) {
> +			tst_res(TPASS, "clock_gettime(2): clock %s passed",
> +				tst_clock_name(tc[i].clktype));
> +		} else if (ret == -1) {
> +			tst_res(TFAIL, "clock_gettime(2): clock %s passed, unchanged timespec",
> +				tst_clock_name(tc[i].clktype));
> +		} else if (ret == -2) {
> +			tst_res(TFAIL, "clock_gettime(2): clock %s passed, Corrupted timespec",
> +				tst_clock_name(tc[i].clktype));
>  		}
>  	}
>  }
> @@ -137,5 +130,7 @@ static void verify_clock_gettime(unsigned int i)
>  static struct tst_test test = {
>  	.test = verify_clock_gettime,
>  	.tcnt = ARRAY_SIZE(tc),
> +	.test_variants = ARRAY_SIZE(variants),
> +	.setup = setup,
>  	.needs_root = 1,
>  };
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime02.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime02.c
> index b4bc6e2d55d4..01bc8e230478 100644
> --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime02.c
> +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime02.c
> @@ -19,10 +19,10 @@
>   */
>  
>  #include "config.h"
> -#include "tst_test.h"
> -#include "lapi/syscalls.h"
> -#include "tst_timer.h"
>  #include "tst_safe_clocks.h"
> +#include "lapi/abisize.h"
> +
> +#include "clock_gettime.h"
>  
>  struct test_case {
>  	clockid_t clktype;
> @@ -30,7 +30,7 @@ struct test_case {
>  	int allow_inval;
>  };
>  
> -struct test_case tc[] = {
> +static struct test_case tc[] = {
>  	{
>  	 .clktype = MAX_CLOCKS,
>  	 .exp_err = EINVAL,
> @@ -81,52 +81,74 @@ struct test_case tc[] = {
>  	 },
>  };
>  
> +#ifdef TST_ABI32
> +static struct timespec spec32;
> +static struct __kernel_old_timespec kspec32;
> +#endif
> +
> +static struct __kernel_timespec kspec64;
> +
>  /*
>   * bad pointer w/ libc causes SIGSEGV signal, call syscall directly
>   */
> -static int sys_clock_gettime(clockid_t clk_id, struct timespec *tp)
> +static struct test_variants {
> +	int (*func)(clockid_t clk_id, void *tp);
> +	void *spec;
> +	int spec_size;
> +	char *desc;
> +} variants[] = {
> +#if defined(TST_ABI32)
> +	{ .func = sys_clock_gettime, .spec = &spec32, .desc = "ABI32 syscall with libc spec"},
> +	{ .func = sys_clock_gettime, .spec = &kspec32, .desc = "ABI32 syscall with kernel spec"},
> +#endif
> +
> +#if defined(TST_ABI64)
> +	{ .func = sys_clock_gettime, .spec = &kspec64, .desc = "ABI64 syscall with kernel spec"},
> +#endif
> +
> +#if (__NR_clock_gettime64 != __LTP__NR_INVALID_SYSCALL)
> +	{ .func = sys_clock_gettime64, .spec = &kspec64, .desc = "ABI64 syscall time64 with kernel spec"},
> +#endif
> +};
> +
> +static void setup(void)
>  {
> -	return tst_syscall(__NR_clock_gettime, clk_id, tp);
> +	tst_res(TINFO, "Testing variant: %d: %s", tst_variant, variants[tst_variant].desc);
>  }
>  
>  static void verify_clock_gettime(unsigned int i)
>  {
> -	struct timespec spec, *specptr;
> -
> -	specptr = &spec;
> +	struct test_variants *tv = &variants[tst_variant];
> +	void *specptr;
>  
>  	/* bad pointer cases */
>  	if (tc[i].exp_err == EFAULT)
>  		specptr = tst_get_bad_addr(NULL);
> +	else
> +		specptr = tv->spec;
>  
> -	TEST(sys_clock_gettime(tc[i].clktype, specptr));
> +	TEST(tv->func(tc[i].clktype, specptr));
>  
> -	if (TST_RET == -1) {
> -
> -		if ((tc[i].exp_err == TST_ERR) ||
> -			(tc[i].allow_inval && TST_ERR == EINVAL)) {
> -
> -			tst_res(TPASS | TTERRNO, "clock_gettime(2): "
> -					"clock %s failed as expected",
> -					tst_clock_name(tc[i].clktype));
> -
> -		} else {
> -
> -			tst_res(TFAIL | TTERRNO, "clock_gettime(2): "
> -					"clock %s failed unexpectedly",
> -					tst_clock_name(tc[i].clktype));
> -		}
> +	if (TST_RET != -1) {
> +		tst_res(TFAIL, "clock_gettime(2): clock %s passed unexcpectedly",
> +			tst_clock_name(tc[i].clktype));
> +		return;
> +	}
>  
> +	if ((tc[i].exp_err == TST_ERR) ||
> +	    (tc[i].allow_inval && TST_ERR == EINVAL)) {
> +		tst_res(TPASS | TTERRNO, "clock_gettime(2): clock %s failed as expected",
> +			tst_clock_name(tc[i].clktype));
>  	} else {
> -
> -		tst_res(TFAIL, "clock_gettime(2): clock %s passed"
> -				" unexcpectedly",
> -				tst_clock_name(tc[i].clktype));
> +		tst_res(TFAIL | TTERRNO, "clock_gettime(2): clock %s failed unexpectedly",
> +			tst_clock_name(tc[i].clktype));
>  	}
>  }
>  
>  static struct tst_test test = {
>  	.test = verify_clock_gettime,
>  	.tcnt = ARRAY_SIZE(tc),
> +	.test_variants = ARRAY_SIZE(variants),
> +	.setup = setup,
>  	.needs_root = 1,
>  };
> diff --git a/testcases/kernel/syscalls/clock_gettime/clock_gettime03.c b/testcases/kernel/syscalls/clock_gettime/clock_gettime03.c
> index cf4706fa0c30..56b5e1983025 100644
> --- a/testcases/kernel/syscalls/clock_gettime/clock_gettime03.c
> +++ b/testcases/kernel/syscalls/clock_gettime/clock_gettime03.c
> @@ -19,9 +19,24 @@
>  
>  #define _GNU_SOURCE
>  #include "tst_safe_clocks.h"
> -#include "tst_timer.h"
>  #include "lapi/namespaces_constants.h"
> -#include "tst_test.h"
> +#include "lapi/abisize.h"
> +
> +#include "clock_gettime.h"
> +
> +static inline long long timespec_diff_ms(void *t1, void *t2)
> +{
> +	struct timespec *ts1 = t1, *ts2 = t2;
> +
> +	return tst_timespec_diff_ms(*ts1, *ts2);
> +}
> +
> +static inline long long timespec64_diff_ms(void *t1, void *t2)
> +{
> +	struct __kernel_timespec *ts1 = t1, *ts2 = t2;
> +
> +	return tst_timespec64_diff_ms(*ts1, *ts2);
> +}

Can't we just cast the function pointer instead?

>  static struct tcase {
>  	int clk_id;
> @@ -38,22 +53,56 @@ static struct tcase {
>  	{CLOCK_MONOTONIC_COARSE, CLOCK_MONOTONIC, 100},
>  };
>  
> -static struct timespec now;
>  static int parent_ns;
>  
> -static void child(struct tcase *tc)
> +#ifdef TST_ABI32
> +static struct timespec spec32_now, spec32_then, spec32_pthen;
> +static struct __kernel_old_timespec kspec32_now, kspec32_then, kspec32_pthen;
> +#endif
> +
> +static struct __kernel_timespec kspec64_now, kspec64_then, kspec64_pthen;
> +
> +static struct test_variants {
> +	int (*func)(clockid_t clk_id, void *tp);
> +	long long (*diff)(void *t1, void *t2);
> +	void *now;
> +	void *then;
> +	void *pthen;
> +	int spec_size;
> +	char *desc;
> +} variants[] = {
> +#if defined(TST_ABI32)
> +	{ .func = libc_clock_gettime, .diff = timespec_diff_ms, .now = &spec32_now, .then = &spec32_then, .pthen = &spec32_pthen, .desc = "ABI32 vDSO or syscall"},
> +	{ .func = sys_clock_gettime, .diff = timespec_diff_ms, .now = &spec32_now, .then = &spec32_then, .pthen = &spec32_pthen, .desc = "ABI32 syscall with libc spec"},
> +	{ .func = sys_clock_gettime, .diff = timespec_diff_ms, .now = &kspec32_now, .then = &kspec32_then, .pthen = &kspec32_pthen, .desc = "ABI32 syscall with kernel spec"},
> +#endif
> +
> +#if defined(TST_ABI64)
> +	{ .func = sys_clock_gettime, .diff = timespec64_diff_ms, .now = &kspec64_now, .then = &kspec64_then, .pthen = &kspec64_pthen, .desc = "ABI64 syscall with kernel spec"},
> +#endif
> +
> +#if (__NR_clock_gettime64 != __LTP__NR_INVALID_SYSCALL)
> +	{ .func = sys_clock_gettime64, .diff = timespec64_diff_ms, .now = &kspec64_now, .then = &kspec64_then, .pthen = &kspec64_pthen, .desc = "ABI64 syscall time64 with kernel spec"},
> +#endif
> +};
> +
> +static void child(struct test_variants *tv, struct tcase *tc)
>  {
> -	struct timespec then;
> -	struct timespec parent_then;
>  	long long diff;
>  
> -	SAFE_CLOCK_GETTIME(tc->clk_id, &then);
> +	if (tv->func(tc->clk_id, tv->then)) {
> +		tst_brk(TBROK | TERRNO, "%d clock_gettime(%s) failed",
> +			__LINE__, tst_clock_name(tc->clk_id));

The __LINE__ is printed by the tst_brk() automatically, no need to
include it twice.

> +	}
>  
>  	SAFE_SETNS(parent_ns, CLONE_NEWTIME);
>  
> -	SAFE_CLOCK_GETTIME(tc->clk_id, &parent_then);
> +	if (tv->func(tc->clk_id, tv->pthen)) {
> +		tst_brk(TBROK | TERRNO, "%d clock_gettime(%s) failed",
> +			__LINE__, tst_clock_name(tc->clk_id));
> +	}
>  
> -	diff = tst_timespec_diff_ms(then, now);
> +	diff = tv->diff(tv->then, tv->now);

I wonder if it was easier if we had a type system. I.e. enum of
different timespec types and function that would dispatch the right
call. If we are going to write a lot of testcases it would be easier
that fiddling with function pointers.

Something as:

enum tst_timespec_type {
	TST_LIBC_TIMESPEC,
	TST_OLD_KERN_TIMESPEC,
	...
};

struct tst_timespec {
	enum tst_timespec_type {
	union {
		struct timespec libc_ts;
		struct __old_kernel_timespec old_kern_ts;
		...
	}
};

static inline long long tst_timespec_diff_ms(struct tst_timespec *ts1, struct tst_timespec ts2)
{
	if (ts1.type != ts2.type)
		tst_brk("Incompatible timespec structures");

	switch (ts1.type) {
	case TST_LIBC_TIMESPEC:
		return tst_timespec_libc_diff(ts1.libc_ts, ts2.libc_ts);
	break;
	...
	}
}

This would allow us to loop over single integer in the testcases. Also
the whole type system could be generated by realtively short perl
script.

>  	if (diff/1000 != tc->off) {
>  		tst_res(TFAIL, "Wrong offset (%s) read %llims",
> @@ -63,7 +112,7 @@ static void child(struct tcase *tc)
>  		        tst_clock_name(tc->clk_id), diff);
>  	}
>  
> -	diff = tst_timespec_diff_ms(parent_then, now);
> +	diff = tv->diff(tv->pthen, tv->now);
>  
>  	if (diff/1000) {
>  		tst_res(TFAIL, "Wrong offset (%s) read %llims",
> @@ -76,6 +125,7 @@ static void child(struct tcase *tc)
>  
>  static void verify_ns_clock(unsigned int n)
>  {
> +	struct test_variants *tv = &variants[tst_variant];
>  	struct tcase *tc = &tcases[n];
>  
>  	SAFE_UNSHARE(CLONE_NEWTIME);
> @@ -83,14 +133,19 @@ static void verify_ns_clock(unsigned int n)
>  	SAFE_FILE_PRINTF("/proc/self/timens_offsets", "%d %d 0",
>  	                 tc->clk_off, tc->off);
>  
> -	SAFE_CLOCK_GETTIME(tc->clk_id, &now);
> +	if (tv->func(tc->clk_id, tv->now)) {
> +		tst_brk(TBROK | TERRNO, "%d clock_gettime(%s) failed",
> +			__LINE__, tst_clock_name(tc->clk_id));
> +	}
>  
>  	if (!SAFE_FORK())
> -		child(tc);
> +		child(tv, tc);
>  }
>  
>  static void setup(void)
>  {
> +	tst_res(TINFO, "Testing variant: %s", variants[tst_variant].desc);
> +
>  	parent_ns = SAFE_OPEN("/proc/self/ns/time_for_children", O_RDONLY);
>  }
>  
> @@ -104,6 +159,7 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.tcnt = ARRAY_SIZE(tcases),
>  	.test = verify_ns_clock,
> +	.test_variants = ARRAY_SIZE(variants),
>  	.needs_root = 1,
>  	.forks_child = 1,
>  	.needs_kconfigs = (const char *[]) {
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
>