[LTP] [PATCH v2 1/2] Add TST_ASSERT_SYSCALL*() macros

[LTP] [PATCH v2 1/2] Add TST_ASSERT_SYSCALL*() macros

[Martin Doucha]

These macros take care of the standard return value checking boilerplate
in cases where the test cannot continue after error.

- TST_ASSERT_SYSCALL() calls tst_brk() if retval != 0
- TST_ASSERT_SYSCALL_FD() calls tst_brk() if retval < 0

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1: Added support for call args pretty printing in error message

 include/tst_test.h | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/include/tst_test.h b/include/tst_test.h
index 8508c2e38..e423ceacc 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -281,6 +281,53 @@ extern void *TST_RET_PTR;
 		TST_ERR = errno; \
 	} while (0)
 
+/* assert that syscall returned only 0 and nothing else */
+#define TST_ASSERT_SYSCALL(SCALL) \
+	TST_ASSERT_SYSCALL_IMPL(SCALL, __FILE__, __LINE__, #SCALL)
+
+#define TST_ASSERT_SYSCALL_IMPL(SCALL, FILENAME, LINENO, CALLSTR, ...) \
+	({ \
+		int _tst_ret; \
+		errno = 0; \
+		_tst_ret = SCALL; \
+		if (_tst_ret == -1) { \
+			int _tst_ttype = errno == ENOTSUP ? TCONF : TBROK; \
+			tst_brk_(FILENAME, LINENO, _tst_ttype | TERRNO, \
+				CALLSTR " failed", ##__VA_ARGS__); \
+		} \
+		if (_tst_ret != 0) { \
+			tst_brk_(FILENAME, LINENO, TBROK | TERRNO, \
+				CALLSTR " returned invalid value %d", \
+				##__VA_ARGS__, _tst_ret); \
+		} \
+		_tst_ret; \
+	})
+
+/*
+ * assert that syscall returned any non-negative value (e.g. valid file
+ * descriptor)
+ */
+#define TST_ASSERT_SYSCALL_FD(SCALL) \
+	TST_ASSERT_SYSCALL_FD_IMPL(SCALL, __FILE__, __LINE__, #SCALL)
+
+#define TST_ASSERT_SYSCALL_FD_IMPL(SCALL, FILENAME, LINENO, CALLSTR, ...) \
+	({ \
+		int _tst_ret; \
+		errno = 0; \
+		_tst_ret = SCALL; \
+		if (_tst_ret == -1) { \
+			int _tst_ttype = errno == ENOTSUP ? TCONF : TBROK; \
+			tst_brk_(FILENAME, LINENO, _tst_ttype | TERRNO, \
+				CALLSTR " failed", ##__VA_ARGS__); \
+		} \
+		if (_tst_ret < 0) { \
+			tst_brk_(FILENAME, LINENO, TBROK | TERRNO, \
+				CALLSTR " returned invalid value %d", \
+				##__VA_ARGS__, _tst_ret); \
+		} \
+		_tst_ret; \
+	})
+
 /*
  * Functions to convert ERRNO to its name and SIGNAL to its name.
  */
-- 
2.25.1

[LTP] [PATCH v2 2/2] Reimplement TST_SAFE_TIMERFD_*() using TST_ASSERT_SYSCALL*()

[Martin Doucha]

Example usage of the TST_ASSERT_SYSCALL*() macros.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v2: Added back argument pretty printing for timerfd_create()

 lib/tst_safe_timerfd.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c
index ffe7b2ef7..8c042f8c8 100644
--- a/lib/tst_safe_timerfd.c
+++ b/lib/tst_safe_timerfd.c
@@ -9,34 +9,18 @@
 #define TST_NO_DEFAULT_MAIN
 #include "tst_test.h"
 
-#define TTYPE (errno == ENOTSUP ? TCONF : TBROK)
-
 int safe_timerfd_create(const char *file, const int lineno,
 				      int clockid, int flags)
 {
-	int fd;
-
-	fd = timerfd_create(clockid, flags);
-	if (fd < 0) {
-		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed",
-			file, lineno, tst_clock_name(clockid));
-	}
-
-	return fd;
+	return TST_ASSERT_SYSCALL_FD_IMPL(timerfd_create(clockid, flags), file,
+		lineno, "timerfd_create(%s)", tst_clock_name(clockid));
 }
 
 int safe_timerfd_gettime(const char *file, const int lineno,
 				int fd, struct itimerspec *curr_value)
 {
-	int rval;
-
-	rval = timerfd_gettime(fd, curr_value);
-	if (rval != 0) {
-		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed",
-			file, lineno);
-	}
-
-	return rval;
+	return TST_ASSERT_SYSCALL_IMPL(timerfd_gettime(fd, curr_value), file,
+		lineno, "timerfd_gettime()");
 }
 
 int safe_timerfd_settime(const char *file, const int lineno,
@@ -44,13 +28,6 @@ int safe_timerfd_settime(const char *file, const int lineno,
 				const struct itimerspec *new_value,
 				struct itimerspec *old_value)
 {
-	int rval;
-
-	rval = timerfd_settime(fd, flags, new_value, old_value);
-	if (rval != 0) {
-		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_settime() failed",
-			file, lineno);
-	}
-
-	return rval;
+	return TST_ASSERT_SYSCALL_IMPL(timerfd_settime(fd, flags, new_value,
+		old_value), file, lineno, "timerfd_settime()");
 }
-- 
2.25.1

[LTP] [PATCH v2 2/2] Reimplement TST_SAFE_TIMERFD_*() using TST_ASSERT_SYSCALL*()

[Cyril Hrubis]

Hi!
>  lib/tst_safe_timerfd.c | 35 ++++++-----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/tst_safe_timerfd.c b/lib/tst_safe_timerfd.c
> index ffe7b2ef7..8c042f8c8 100644
> --- a/lib/tst_safe_timerfd.c
> +++ b/lib/tst_safe_timerfd.c
> @@ -9,34 +9,18 @@
>  #define TST_NO_DEFAULT_MAIN
>  #include "tst_test.h"
>  
> -#define TTYPE (errno == ENOTSUP ? TCONF : TBROK)
> -
>  int safe_timerfd_create(const char *file, const int lineno,
>  				      int clockid, int flags)
>  {
> -	int fd;
> -
> -	fd = timerfd_create(clockid, flags);
> -	if (fd < 0) {
> -		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed",
> -			file, lineno, tst_clock_name(clockid));
> -	}
> -
> -	return fd;
> +	return TST_ASSERT_SYSCALL_FD_IMPL(timerfd_create(clockid, flags), file,
> +		lineno, "timerfd_create(%s)", tst_clock_name(clockid));
>  }

Well at this point we basically pass everything that has been in the
function to the macro. So we do not save much on typing, the code is not
simpler and the obvious and readable code we had has been turned into a
macro. I do not think that this is improvement.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] Reimplement TST_SAFE_TIMERFD_*() using TST_ASSERT_SYSCALL*()

[Petr Vorel]

Hi Martin,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> -	fd = timerfd_create(clockid, flags);
> -	if (fd < 0) {
> -		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_create(%s) failed",
> -			file, lineno, tst_clock_name(clockid));
> -	}
> -
> -	return fd;
> +	return TST_ASSERT_SYSCALL_FD_IMPL(timerfd_create(clockid, flags), file,
> +		lineno, "timerfd_create(%s)", tst_clock_name(clockid));
>  }

>  int safe_timerfd_gettime(const char *file, const int lineno,
>  				int fd, struct itimerspec *curr_value)
>  {
> -	int rval;
> -
> -	rval = timerfd_gettime(fd, curr_value);
> -	if (rval != 0) {
> -		tst_brk(TTYPE | TERRNO, "%s:%d timerfd_gettime() failed",
> -			file, lineno);
> -	}
> -
> -	return rval;
> +	return TST_ASSERT_SYSCALL_IMPL(timerfd_gettime(fd, curr_value), file,
> +		lineno, "timerfd_gettime()");
>  }

I like sprintf formatting (it's needed), but it'd be nice to have a way to avoid
it, when it's just foo() (using SCALL_FUN and SCALL_PARAMS).
But that's probably too much optimization, resulting in unreadable macros.

Kind regards,
Petr

[LTP] [PATCH v2 1/2] Add TST_ASSERT_SYSCALL*() macros

[Petr Vorel]

Hi Martin,

> These macros take care of the standard return value checking boilerplate
> in cases where the test cannot continue after error.

> - TST_ASSERT_SYSCALL() calls tst_brk() if retval != 0
> - TST_ASSERT_SYSCALL_FD() calls tst_brk() if retval < 0
Reviewed-by: Petr Vorel <pvorel@suse.cz>

What I like on these macros (besides DRY) is that it really shows the test, not
the library, see
before:
tst_safe_timerfd.c:21: BROK: timerfd01.c:89 timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)
after:
timerfd01.c:89: BROK: timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)

> +/* assert that syscall returned only 0 and nothing else */
> +#define TST_ASSERT_SYSCALL(SCALL) \
> +	TST_ASSERT_SYSCALL_IMPL(SCALL, __FILE__, __LINE__, #SCALL)
> +
> +#define TST_ASSERT_SYSCALL_IMPL(SCALL, FILENAME, LINENO, CALLSTR, ...) \
> +	({ \
> +		int _tst_ret; \
> +		errno = 0; \
> +		_tst_ret = SCALL; \
> +		if (_tst_ret == -1) { \
> +			int _tst_ttype = errno == ENOTSUP ? TCONF : TBROK; \
> +			tst_brk_(FILENAME, LINENO, _tst_ttype | TERRNO, \
> +				CALLSTR " failed", ##__VA_ARGS__); \
> +		} \
> +		if (_tst_ret != 0) { \
> +			tst_brk_(FILENAME, LINENO, TBROK | TERRNO, \
> +				CALLSTR " returned invalid value %d", \
> +				##__VA_ARGS__, _tst_ret); \
> +		} \
> +		_tst_ret; \
> +	})
> +
> +/*
> + * assert that syscall returned any non-negative value (e.g. valid file
> + * descriptor)
> + */
> +#define TST_ASSERT_SYSCALL_FD(SCALL) \
> +	TST_ASSERT_SYSCALL_FD_IMPL(SCALL, __FILE__, __LINE__, #SCALL)
> +
> +#define TST_ASSERT_SYSCALL_FD_IMPL(SCALL, FILENAME, LINENO, CALLSTR, ...) \
> +	({ \
> +		int _tst_ret; \
> +		errno = 0; \
> +		_tst_ret = SCALL; \
> +		if (_tst_ret == -1) { \
> +			int _tst_ttype = errno == ENOTSUP ? TCONF : TBROK; \
> +			tst_brk_(FILENAME, LINENO, _tst_ttype | TERRNO, \
> +				CALLSTR " failed", ##__VA_ARGS__); \
> +		} \
> +		if (_tst_ret < 0) { \
> +			tst_brk_(FILENAME, LINENO, TBROK | TERRNO, \
> +				CALLSTR " returned invalid value %d", \
> +				##__VA_ARGS__, _tst_ret); \
> +		} \
> +		_tst_ret; \
> +	})

I'd consider to add single implementation, which would be influenced with flags
Something like
if ((flags & TST_ASSERT_LT_0) && _tst_ret < 0 || _tst_ret != 0) \

But maybe some people will not like degreased readability with macro,
and using flags would make it even a bit harder to read.

Kind regards,
Petr

[LTP] [PATCH v2 1/2] Add TST_ASSERT_SYSCALL*() macros

[Cyril Hrubis]

Hi!
> What I like on these macros (besides DRY) is that it really shows the test, not
> the library, see
> before:
> tst_safe_timerfd.c:21: BROK: timerfd01.c:89 timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)
> after:
> timerfd01.c:89: BROK: timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)

That's because it calls tst_brk_() correctly instead of tst_brk(). I
should have caught that during the review.

Also given the way it's structured now we can pass these parameters to a
shell script as well and generate the end result easily. With a bit more
work we can generate both header and C source as well and would still
prefer that over these macros.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] Add TST_ASSERT_SYSCALL*() macros

[Petr Vorel]

Hi,

> > What I like on these macros (besides DRY) is that it really shows the test, not
> > the library, see
> > before:
> > tst_safe_timerfd.c:21: BROK: timerfd01.c:89 timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)
> > after:
> > timerfd01.c:89: BROK: timerfd_create(CLOCK_MONOTONIC) failed: EINVAL (22)

> That's because it calls tst_brk_() correctly instead of tst_brk(). I
> should have caught that during the review.
OK. Again, sorry for that error.

> Also given the way it's structured now we can pass these parameters to a
> shell script as well and generate the end result easily. With a bit more
> work we can generate both header and C source as well and would still
> prefer that over these macros.
OK, readability wins (I agree).

Kind regards,
Petr