[v8,2/6] tests: support waitpid

Submitted by Leonid Bobrov on Feb. 27, 2019, 7:13 p.m.

Details

Message ID 20190227191313.66300-2-mazocomp@disroot.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Leonid Bobrov Feb. 27, 2019, 7:13 p.m.
From: Vadim Zhukov <persgray@gmail.com>

*BSD don't have waitid()

Co-authored-by: Koop Mast <kwm@rainbow-runner.nl>
Signed-off-by: Leonid Bobrov <mazocomp@disroot.org>
---
 configure.ac            |  4 ++++
 tests/test-compositor.c | 25 +++++++++++++++++++++++--
 tests/test-runner.c     | 29 ++++++++++++++++++++++++++++-
 3 files changed, 55 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 495e1a6..c0f1c37 100644
--- a/configure.ac
+++ b/configure.ac
@@ -65,6 +65,10 @@  AC_SUBST(GCC_CFLAGS)
 AC_CHECK_HEADERS([sys/prctl.h])
 AC_CHECK_FUNCS([accept4 mkostemp posix_fallocate prctl])
 
+# waitid() and signal.h are needed for the test suite.
+AC_CHECK_FUNCS([waitid])
+AC_CHECK_HEADERS([signal.h])
+
 AC_ARG_ENABLE([libraries],
 	      [AC_HELP_STRING([--disable-libraries],
 			      [Disable compilation of wayland libraries])],
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
index 72f6351..6e12630 100644
--- a/tests/test-compositor.c
+++ b/tests/test-compositor.c
@@ -23,6 +23,8 @@ 
  * SOFTWARE.
  */
 
+#include "config.h"
+
 #include <assert.h>
 #include <errno.h>
 #include <stdio.h>
@@ -86,8 +88,8 @@  get_socket_name(void)
 	static char retval[64];
 
 	gettimeofday(&tv, NULL);
-	snprintf(retval, sizeof retval, "wayland-test-%d-%ld%ld",
-		 getpid(), tv.tv_sec, tv.tv_usec);
+	snprintf(retval, sizeof retval, "wayland-test-%d-%lld%lld",
+		 getpid(), (long long)tv.tv_sec, (long long)tv.tv_usec);
 
 	return retval;
 }
@@ -97,10 +99,15 @@  handle_client_destroy(void *data)
 {
 	struct client_info *ci = data;
 	struct display *d;
+#ifdef HAVE_WAITID
 	siginfo_t status;
+#else
+	int istatus;
+#endif
 
 	d = ci->display;
 
+#ifdef HAVE_WAITID
 	assert(waitid(P_PID, ci->pid, &status, WEXITED) != -1);
 
 	switch (status.si_code) {
@@ -118,6 +125,20 @@  handle_client_destroy(void *data)
 		ci->exit_code = status.si_status;
 		break;
 	}
+#else
+	assert(waitpid(ci->pid, &istatus, WNOHANG) != -1);
+
+	if (WIFSIGNALED(istatus)) {
+		fprintf(stderr, "Client '%s' was killed by signal %d\n",
+			ci->name, WTERMSIG(istatus));
+		ci->exit_code = WEXITSTATUS(istatus);
+	} else if (WIFEXITED(istatus)) {
+		if (WEXITSTATUS(istatus) != EXIT_SUCCESS)
+			fprintf(stderr, "Client '%s' exited with code %d\n",
+				ci->name, WEXITSTATUS(istatus));
+		ci->exit_code = WEXITSTATUS(istatus);
+	}
+#endif
 
 	++d->clients_terminated_no;
 	if (d->clients_no == d->clients_terminated_no) {
diff --git a/tests/test-runner.c b/tests/test-runner.c
index 1487dc4..7fa72eb 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -23,6 +23,8 @@ 
  * SOFTWARE.
  */
 
+#include "config.h"
+
 #define _GNU_SOURCE
 
 #include <unistd.h>
@@ -32,6 +34,7 @@ 
 #include <sys/wait.h>
 #include <sys/stat.h>
 #include <string.h>
+#include <signal.h>
 #include <assert.h>
 #include <dlfcn.h>
 #include <errno.h>
@@ -293,7 +296,11 @@  int main(int argc, char *argv[])
 	const struct test *t;
 	pid_t pid;
 	int total, pass;
+#ifdef HAVE_WAITID
 	siginfo_t info;
+#else
+	int status;
+#endif
 
 	if (isatty(fileno(stderr)))
 		is_atty = 1;
@@ -336,7 +343,8 @@  int main(int argc, char *argv[])
 		if (pid == 0)
 			run_test(t); /* never returns */
 
-		if (waitid(P_PID, pid, &info, WEXITED)) {
+#ifdef HAVE_WAITID
+		if (waitid(P_PID, 0, &info, WEXITED)) {
 			stderr_set_color(RED);
 			fprintf(stderr, "waitid failed: %m\n");
 			stderr_reset_color();
@@ -367,6 +375,25 @@  int main(int argc, char *argv[])
 
 			break;
 		}
+#else
+		if (waitpid(-1, &status, 0) == -1) {
+			fprintf(stderr, "waitpid failed: %s\n",
+				strerror(errno));
+			abort();
+		}
+
+		fprintf(stderr, "test \"%s\":\t", t->name);
+		if (WIFEXITED(status)) {
+			fprintf(stderr, "exit status %d", WEXITSTATUS(status));
+			if (WEXITSTATUS(status) == EXIT_SUCCESS)
+				success = 1;
+			} else if (WIFSIGNALED(status)) {
+				fprintf(stderr, "signal %d", WTERMSIG(status));
+			}
+#endif
+
+		if (t->must_fail)
+			success = !success;
 
 		if (success) {
 			pass++;

Comments

On Wed, 27 Feb 2019 21:13:09 +0200
Leonid Bobrov <mazocomp@disroot.org> wrote:

> From: Vadim Zhukov <persgray@gmail.com>
> 
> *BSD don't have waitid()

Hi Leonid,

many details to fix here. This patch breaks all tests that use the
runner on Linux.

> 
> Co-authored-by: Koop Mast <kwm@rainbow-runner.nl>
> Signed-off-by: Leonid Bobrov <mazocomp@disroot.org>
> ---
>  configure.ac            |  4 ++++
>  tests/test-compositor.c | 25 +++++++++++++++++++++++--
>  tests/test-runner.c     | 29 ++++++++++++++++++++++++++++-
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 495e1a6..c0f1c37 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -65,6 +65,10 @@ AC_SUBST(GCC_CFLAGS)
>  AC_CHECK_HEADERS([sys/prctl.h])
>  AC_CHECK_FUNCS([accept4 mkostemp posix_fallocate prctl])
>  
> +# waitid() and signal.h are needed for the test suite.
> +AC_CHECK_FUNCS([waitid])

Should you not check for waitpid as well, to error out on systems where
neither is present rather than assuming that lack of waitid means
waitpid is present?

> +AC_CHECK_HEADERS([signal.h])

The result for the signal.h check does not seem to be used at all.
Should it not?

> +
>  AC_ARG_ENABLE([libraries],
>  	      [AC_HELP_STRING([--disable-libraries],
>  			      [Disable compilation of wayland libraries])],
> diff --git a/tests/test-compositor.c b/tests/test-compositor.c
> index 72f6351..6e12630 100644
> --- a/tests/test-compositor.c
> +++ b/tests/test-compositor.c
> @@ -23,6 +23,8 @@
>   * SOFTWARE.
>   */
>  
> +#include "config.h"
> +
>  #include <assert.h>
>  #include <errno.h>
>  #include <stdio.h>
> @@ -86,8 +88,8 @@ get_socket_name(void)
>  	static char retval[64];
>  
>  	gettimeofday(&tv, NULL);
> -	snprintf(retval, sizeof retval, "wayland-test-%d-%ld%ld",
> -		 getpid(), tv.tv_sec, tv.tv_usec);
> +	snprintf(retval, sizeof retval, "wayland-test-%d-%lld%lld",
> +		 getpid(), (long long)tv.tv_sec, (long long)tv.tv_usec);

This hunk does not seem to have anything to do with waitid, hence does
not belong in this patch.

>  
>  	return retval;
>  }
> @@ -97,10 +99,15 @@ handle_client_destroy(void *data)
>  {
>  	struct client_info *ci = data;
>  	struct display *d;
> +#ifdef HAVE_WAITID
>  	siginfo_t status;
> +#else
> +	int istatus;
> +#endif
>  
>  	d = ci->display;
>  
> +#ifdef HAVE_WAITID
>  	assert(waitid(P_PID, ci->pid, &status, WEXITED) != -1);

Would it not be easier to rewrite this code to use waitpid() instead?

>  
>  	switch (status.si_code) {
> @@ -118,6 +125,20 @@ handle_client_destroy(void *data)
>  		ci->exit_code = status.si_status;
>  		break;
>  	}
> +#else
> +	assert(waitpid(ci->pid, &istatus, WNOHANG) != -1);

Why WNOHANG? We do want to wait for the child to exit if it hasn't
already.

> +
> +	if (WIFSIGNALED(istatus)) {
> +		fprintf(stderr, "Client '%s' was killed by signal %d\n",
> +			ci->name, WTERMSIG(istatus));
> +		ci->exit_code = WEXITSTATUS(istatus);
> +	} else if (WIFEXITED(istatus)) {
> +		if (WEXITSTATUS(istatus) != EXIT_SUCCESS)
> +			fprintf(stderr, "Client '%s' exited with code %d\n",
> +				ci->name, WEXITSTATUS(istatus));
> +		ci->exit_code = WEXITSTATUS(istatus);
> +	}
> +#endif

Btw. such big or multiple #ifdef blocks inside a function that also has
common stuff make understanding it more difficult. If such alternate
paths must exist, they would be better as a separate function whose
whole body is switched.

>  
>  	++d->clients_terminated_no;
>  	if (d->clients_no == d->clients_terminated_no) {
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 1487dc4..7fa72eb 100644
> --- a/tests/test-runner.c
> +++ b/tests/test-runner.c
> @@ -23,6 +23,8 @@
>   * SOFTWARE.
>   */
>  
> +#include "config.h"
> +
>  #define _GNU_SOURCE
>  
>  #include <unistd.h>
> @@ -32,6 +34,7 @@
>  #include <sys/wait.h>
>  #include <sys/stat.h>
>  #include <string.h>
> +#include <signal.h>
>  #include <assert.h>
>  #include <dlfcn.h>
>  #include <errno.h>
> @@ -293,7 +296,11 @@ int main(int argc, char *argv[])
>  	const struct test *t;
>  	pid_t pid;
>  	int total, pass;
> +#ifdef HAVE_WAITID
>  	siginfo_t info;
> +#else
> +	int status;
> +#endif
>  
>  	if (isatty(fileno(stderr)))
>  		is_atty = 1;
> @@ -336,7 +343,8 @@ int main(int argc, char *argv[])
>  		if (pid == 0)
>  			run_test(t); /* never returns */
>  
> -		if (waitid(P_PID, pid, &info, WEXITED)) {
> +#ifdef HAVE_WAITID
> +		if (waitid(P_PID, 0, &info, WEXITED)) {

Why change this?

Could this code not be rewritten to use waitpid() instead of waitid()?

>  			stderr_set_color(RED);
>  			fprintf(stderr, "waitid failed: %m\n");
>  			stderr_reset_color();
> @@ -367,6 +375,25 @@ int main(int argc, char *argv[])
>  
>  			break;
>  		}
> +#else
> +		if (waitpid(-1, &status, 0) == -1) {

Why not wait for the right pid?

> +			fprintf(stderr, "waitpid failed: %s\n",
> +				strerror(errno));
> +			abort();
> +		}
> +
> +		fprintf(stderr, "test \"%s\":\t", t->name);
> +		if (WIFEXITED(status)) {

Looks like an unbalanced opening brace.

> +			fprintf(stderr, "exit status %d", WEXITSTATUS(status));

Color coding of the prints is missing.

> +			if (WEXITSTATUS(status) == EXIT_SUCCESS)
> +				success = 1;
> +			} else if (WIFSIGNALED(status)) {
> +				fprintf(stderr, "signal %d", WTERMSIG(status));
> +			}
> +#endif
> +
> +		if (t->must_fail)
> +			success = !success;

Having this in the common path does not seem right.

>  
>  		if (success) {
>  			pass++;


Thanks,
pq
You know, I think this whole patch is silly, because waitid() is part of
POSIX, FreeBSD and NetBSD implement it while OpenBSD and DragonFly BSD
don't. I'll ask OpenBSD upstream to merge NetBSD's implementation and
DragonFly BSD upstream to merge FreeBSD's implementation.

OpenBSD aims standardization so we should blame OpenBSD.