[PATCH v8 2/6] tests: support waitpid

[Pekka Paalanen]

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

> From: Vadim Zhukov <persgray at 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 at rainbow-runner.nl>
> Signed-off-by: Leonid Bobrov <mazocomp at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190304/4777751b/attachment.sig>