On Wed, 27 Feb 2019 21:13:09 +0200 Leonid Bobrov <mazoc...@disroot.org> wrote:
> From: Vadim Zhukov <persg...@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 <k...@rainbow-runner.nl>
> Signed-off-by: Leonid Bobrov <mazoc...@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
pgpwUdm9HX0rS.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
