[PATCH weston v2] tests: Adding simple waycheck validation tool.
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 25 16:06:55 UTC 2016
On Wed, 23 Dec 2015 22:26:19 -0800
"Jon A. Cruz" <jonc at osg.samsung.com> wrote:
> Adds an initial implementation of a testing tool that uses the unit
> test framework to run checks against an arbitrary Wayland compositor.
> Note that this is not intended for Weston-specific testing, but for
> generic Wayland testing.
>
> Signed-off-by: Jon A. Cruz <jonc at osg.samsung.com>
>
> ---
>
> Changes from v1:
>
> - Added test fixture to launch compositor used for tests.
> - Hooked waycheck into "make check"
> - Corrected problem in version binding that was inherited from legacy
> test code.
> - Incorporated misc fixes from code review feedback.
> - Removed files duplicated in rebasing.
Hi Jon,
I see my comments on the previous version have been mostly addressed.
Some of those were seemingly ignored, like dynamically allocating the
wl_registry listener struct, you could shrug those off by just
explaining (in email is fine) why things are so. I probably missed
something.
I am very glad to see 'make check' now launches weston and runs
waycheck.
You intended waycheck to be a compositor-agnostic test program. It will
need a command line option to not launch weston and connect to an
existing compositor instead. Something like
--target=[weston|running-display|...] with 'weston' being the default.
That can be left for follow-ups, though.
The compositor launcher fixture is an interesting piece of code, and I
have specific comments about it inline.
It's good to have waycheck use the compositor launcher fixture, so that
we can eventually get rid of all the old stuff. Having just a single
way to launch weston correctly is a good thing.
One important thing missing is an explanation on whether or which
existing tests waycheck reimplements.
As a general comment, there are many many cases where errors are
handled gracefully when they should cause an explosion instead. For
instance, wtst_ctx_private_destroy() silently accepts a NULL argument.
It would be better to explode by default, and if we happen to find a
legitimate case where really want e.g. a bad argument to be ignored, do
that only then. It's a test suite, it's supposed to explode on the
slightest.
It would also be perfectly fine to use xzalloc() instead of any other
*alloc() that does not guarantee success. malloc() failing is so
fundamental that you can just yell and fail immediately. A test
suite must fail hard, if it runs out of memory.
Some notes that might help people to read this, that I think I'll put
up as docs somewhere:
tools/wayland_fixtures/inc/wtst_fixtures.h corresponds to
tests/weston-test-client-helper.h, and the .c files too. These are the
fixture that implements a test client framework.
The compositor launching framework is in wtst_compfixture.[ch].
An example of using both frameworks is in waycheck.c.
>
> ---
> .gitignore | 1 +
> Makefile.am | 52 +-
> doc/doxygen/devtools.dox | 10 +-
> doc/doxygen/tooldev.doxygen.in | 3 +-
> doc/doxygen/tools.dox | 1 +
> doc/doxygen/tools.doxygen.in | 6 +-
> tools/waycheck/rough_draw.c | 285 ++++++
> tools/waycheck/rough_draw.h | 49 +
> tools/waycheck/waycheck.c | 457 +++++++++
> tools/waycheck/waycheck.dox | 29 +
> tools/wayland_fixtures/inc/wtst_compfixture.h | 76 ++
> tools/wayland_fixtures/inc/wtst_fixtures.h | 268 ++++++
> tools/wayland_fixtures/src/wtst_compfixture.c | 411 ++++++++
> tools/wayland_fixtures/src/wtst_fixtures.c | 1238 +++++++++++++++++++++++++
> 14 files changed, 2875 insertions(+), 11 deletions(-)
> create mode 100644 tools/waycheck/rough_draw.c
> create mode 100644 tools/waycheck/rough_draw.h
> create mode 100644 tools/waycheck/waycheck.c
> create mode 100644 tools/waycheck/waycheck.dox
> create mode 100644 tools/wayland_fixtures/inc/wtst_compfixture.h
> create mode 100644 tools/wayland_fixtures/inc/wtst_fixtures.h
> create mode 100644 tools/wayland_fixtures/src/wtst_compfixture.c
> create mode 100644 tools/wayland_fixtures/src/wtst_fixtures.c
>
> diff --git a/Makefile.am b/Makefile.am
> index cbb3b57..72f131e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1320,6 +1339,27 @@ zuctest_SOURCES = \
> tools/zunitc/test/fixtures_test.c \
> tools/zunitc/test/zunitc_test.c
>
> +noinst_PROGRAMS += waycheck$(EXEEXT)
> +
> +waycheck_LDADD = \
> + $(TEST_CLIENT_LIBS) \
> + libwayland-fixtures.la \
> + libshared.la
> +
> +waycheck_CFLAGS = \
> + -I$(top_srcdir)/tools/zunitc/inc \
> + -I$(top_srcdir)/tools/wayland_fixtures/inc \
> + -I$(top_builddir)/clients \
I built this and I still don't see any headers in
$(top_builddir)/clients, shouldn't this -I be dropped?
> + -I$(top_srcdir)/shared \
> + $(TEST_CLIENT_CFLAGS) \
> + $(AM_CFLAGS)
> +
> +waycheck_SOURCES = \
> + tools/waycheck/waycheck.c \
> + tools/waycheck/rough_draw.c \
> + tools/waycheck/rough_draw.h \
> + shared/helpers.h
> +
> EXTRA_DIST += \
> tests/weston-tests-env \
> tests/internal-screenshot.ini \
> diff --git a/tools/waycheck/rough_draw.c b/tools/waycheck/rough_draw.c
> new file mode 100644
> index 0000000..ae2dd33
> --- /dev/null
> +++ b/tools/waycheck/rough_draw.c
> @@ -0,0 +1,285 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + */
> +
> +/**
> + * @file simple drawing for test.
> + *
> + * @todo refactor to use pixman instead of direct manipulation.
I suppose this comment also implies that this code is broken on
big-endian systems. Could mention that this is likely the case.
Replacing the code with Pixman calls is really the right thing to do
here.
> diff --git a/tools/waycheck/waycheck.c b/tools/waycheck/waycheck.c
> new file mode 100644
> index 0000000..b0420c0
> --- /dev/null
> +++ b/tools/waycheck/waycheck.c
> @@ -0,0 +1,457 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + */
> +
> +#include "config.h"
> +/**
> + * Set up environment and fixture data for launching weston.
> + */
> +static void
> +prep_for_weston(int argc, char* argv[])
> +{
> + char *path = NULL;
> + char *params[] = {
> + "--backend=headless-backend.so",
> + NULL
> + };
> +
> + wtst_comp_fxt_set_environment("weston",
> + argc > 0 ? argv[0] : NULL,
> + getenv("PATH"));
Using $PATH is wrong, this will look for the installed weston, while
the weston we want to test is in the build directory. It's the same
with all binaries and files, they must all come from the build or
source directories. We cannot require 'make install' before 'make
check'.
Oh wait, from the doc I assumed wrong what
wtst_comp_fxt_set_environment() does.
> +
> + /* Certain Weston internal code uses this variable */
> + path = wtst_comp_fxt_get_absbase();
> + setenv("WESTON_BUILD_DIR", path, 1);
> + free(path);
Setting WESTON_BUILD_DIR should probably be internal to the compositor
launcher fixture. Weston will always need it, and it won't launch
anything but weston.
That said, I don't think we even need wtst_comp_fxt_set_environment()
in the API.
> +
> + wtst_comp_fxt_set_params("waycheck", "--log", "--socket", params);
> +}
> +
> +/*
> + * Since we need to hook argv[0] to check on runtime path, we can not use
Can't you use zuc_get_program_name() from setup_suite_config()?
> + * the stock zunitc main() lib.
> + * This is copied from that with the needed addition of grabbing the name of
> + * the executable.
> + */
> +int
> +main(int argc, char* argv[])
> +{
> + bool helped = false;
> + int rc = zuc_initialize(&argc, argv, &helped);
> +
> + /* TODO add command-line param to allow overriding the name. */
> + /* This next line is the only addition to the stock zunitc main. */
> + prep_for_weston(argc, argv);
> +
> + if ((rc == EXIT_SUCCESS) && !helped) {
> + /* Stop if any unrecognized parameters were encountered. */
> + if (argc > 1) {
> + printf("%s: unrecognized option '%s'\n",
> + argv[0], argv[1]);
> + printf("Try '%s --help' for more information.\n",
> + argv[0]);
> + rc = EXIT_FAILURE;
> + } else {
> + rc = ZUC_RUN_TESTS();
> + }
> + }
> +
> + zuc_cleanup();
> + return rc;
> +}
> diff --git a/tools/wayland_fixtures/inc/wtst_compfixture.h b/tools/wayland_fixtures/inc/wtst_compfixture.h
> new file mode 100644
> index 0000000..f8c9868
> --- /dev/null
> +++ b/tools/wayland_fixtures/inc/wtst_compfixture.h
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + */
> +
> +#ifndef WTST_COMP_FIXTURE_H
> +#define WTST_COMP_FIXTURE_H
> +
> +/**
> + * @file
> + * Simple fixture to ensure a proper compositor is running.
> + */
> +
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * Simple fixture to launch the Wayland compositor to test, defaulting to
> + * weston.
> + */
> +struct wtst_comp_fxt {
> + /**
> + * Name of wayland socket used for this instance.
> + * Note that this may be NULL if the defaults are to be used.
> + */
> + const char *sockname;
> +
> + /** Context for internal implementation details. */
> + struct wtst_comp_fxt_private *private;
> +};
> +
> +struct wtst_comp_fxt *
> +wtst_comp_fxt_create(void);
> +
> +void
> +wtst_comp_fxt_destroy(struct wtst_comp_fxt *fxt);
> +
> +void
> +wtst_comp_fxt_set_environment(const char *compname, const char *argv0,
> + const char *path);
> +
> +char *
> +wtst_comp_fxt_get_absbase(void);
> +
> +void
> +wtst_comp_fxt_set_params(const char *log_name, const char *log_param,
> + const char *sock_param, char **params);
I have a big problem with this API. It seems to be backwards. I would
assume I start with wtst_comp_fxt_create() and then set up the
parameters with the rest, but it actually works the opposite. create()
is supposed to be called the last thing as it launches weston. This
also makes you rely a lot on global data in wtst_compfixture.c, which
is not nice.
I think this API should be revised to work like this:
1. wtst_comp_fxt_create()
2. set parameters
3. wtst_comp_fxt_start() to launch weston
4. tests get executed...
5. wtst_comp_fxt_destroy() to cause weston to exit if it has not exited
yet.
With a few details:
wtst_comp_fxt_start() must not return until weston is up and ready.
Otherwise the test client may start before weston can accept
connections, and fails due to a race. This means you need some
signalling with the compositor to have it signal readyness. The
systemd-notify plugin does the signalling to systemd for instance, but
the way it does it may be too early if we want to e.g. see a wallpaper
in a screenshot. I see that you are testing the socket connectability,
that should be good enough for now.
wtst_comp_fxt_destroy() must also wait for the compositor process to
exit, and inspect its exit status to see if the compositor crashed or
anything. If the compositor ever crashes or exits unexpectedly, the
test must be marked failed.
If the compositor crashes while running a bunch of cases, is it
possibly to pinpoint which case triggered the crash?
The old test suite solved these issues with the test plugin. We have
the horrible weston-tests-env script, which launches weston with the
test plugin. Then the test plugin launches the client test program and
monitors its exit. The client program runs and exits with a status. The
test plugin gets the status and causes weston to exit with a similar
status. This way the client exit status gets passed all the way up to
the automake test harness.
I assume you want to fork off weston and then fork some tests as
"sibling" processes, rather fork the actual test as a child to weston.
> +
> +
> +#ifdef __cplusplus
> +} /* extern "C" */
> +#endif
> +
> +#endif /* WTST_COMP_FIXTURE_H */
> diff --git a/tools/wayland_fixtures/src/wtst_compfixture.c b/tools/wayland_fixtures/src/wtst_compfixture.c
> new file mode 100644
> index 0000000..02f80b0
> --- /dev/null
> +++ b/tools/wayland_fixtures/src/wtst_compfixture.c
> @@ -0,0 +1,411 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + */
> +
> +#include "config.h"
> +
> +#include "wtst_compfixture.h"
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/un.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "shared/zalloc.h"
> +#include "zunitc/zunitc.h"
> +
> +#define MAX_ARGS 256
> +
> +#define MAX_SOCK_PARAM_LEN 36
> +
> +#define NS_PER_MS 1000000
> +#define WAIT_DURATION_MS 2000
> +
> +static char g_compname[NAME_MAX] = {0};
> +static char g_basepath[PATH_MAX] = {0};
> +static char *g_abs_basepath = NULL;
> +static char *g_pathenv = NULL;
> +static char *g_sock_param = NULL;
> +static char *g_log_param = NULL;
> +static char *g_log_name = NULL;
> +static char **g_extra_params = NULL;
That's a lot of globals. :-/
> +struct wtst_comp_fxt_private {
> + pid_t pid;
> + char sockname[MAX_SOCK_PARAM_LEN];
> +};
> +
> +static struct wtst_comp_fxt_private *
> +wtst_comp_fxt_private_create(void)
> +{
> + struct wtst_comp_fxt_private *private = zalloc(sizeof(*private));
> +
> + ZUC_ASSERTG_NOT_NULL(private, out);
> + private->pid = -1;
> +
> +out:
> + return private;
> +}
> +
> +static void
> +wtst_comp_fxt_private_destroy(struct wtst_comp_fxt_private *private)
> +{
> + if (!private)
> + return;
> +
> + if (private->pid != -1)
> + kill(private->pid, SIGTERM);
> +
> + free(private);
> +}
> +
> +/**
> + * Helper function to create a common name based on the passed in pid.
> + *
> + * @param str the output buffer to write the string to.
> + * @param strlen the size of the output buffer.
> + * @param id the pid to base the name on.
> + */
> +static void
> +populate_sockname(char *str, size_t strlen, pid_t id)
> +{
> + snprintf(str, strlen, "WAYLANDTEST_%u", (unsigned int)id);
> +}
> +
> +static void
> +create_logdir(void)
> +{
> + char dir[PATH_MAX] = {0};
> + snprintf(dir, sizeof(dir), "%s/logs", g_abs_basepath);
> + mkdir(dir, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
> +}
> +
> +/**
> + * Helper to wait for a local socket with a given name to become available.
> + *
> + * @param sockname the socket name to connect to.
> + */
> +static void
> +wait_for_local_socket(const char *sockname)
> +{
> + socklen_t size = 0;
> + int name_size = 0;
> + int fd = -1;
> + int rc = 0;
> + int code = 0;
> + struct timespec ts = {.tv_nsec = 1 * NS_PER_MS};
> + struct sockaddr_un addr = {.sun_family = AF_LOCAL};
> + const char *runtime_dir = getenv("XDG_RUNTIME_DIR");
> +
> + if (!runtime_dir) {
> + return;
> + }
> + name_size = snprintf(addr.sun_path, sizeof(addr.sun_path),
> + "%s/%s", runtime_dir, sockname) + 1;
> + if (name_size > (int)sizeof(addr.sun_path))
> + return;
> + size = offsetof(struct sockaddr_un, sun_path) + name_size;
> +
> + fd = socket(AF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0);
> + if (fd < 0) {
> + int flags = 0;
> + if (errno != EINVAL)
> + return;
> + fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> + if (fd < 0)
> + return;
> + flags = fcntl(fd, F_GETFD);
> + if (flags == -1)
> + goto exit;
> + if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == -1)
> + goto exit;
> + }
So that's an open-coded wl_os_socket_cloexec(). We should copy that
helper from Wayland to shared/os-compatibility.[ch].
> +
> + rc = 0;
> + do {
> + if (rc == -1) {
> + nanosleep(&ts, NULL);
> + ts.tv_nsec *= 2;
> + }
> + errno = 0;
> + rc = connect(fd, (struct sockaddr *)&addr, size);
> + code = errno;
> + } while ((rc == -1)
> + && (ts.tv_nsec < WAIT_DURATION_MS * NS_PER_MS)
> + && ((code == ENOENT) || (code == EAGAIN) || (code == EINTR)));
Huh, didn't even think of that. Cool, I suppose.
> +
> + exit:
> + close(fd);
> +}
> +
> +/**
> + * Creates an instance of the compositor-launch test fixture.
> + *
> + * @return a pointer to a test fixture upon success, NULL otherwise.
> + * @see wtst_comp_fxt_destroy()
> + */
> +struct wtst_comp_fxt *
> +wtst_comp_fxt_create(void)
> +{
> + pid_t pid = -1;
> +
> + struct wtst_comp_fxt *fxt = zalloc(sizeof(*fxt));
> + ZUC_ASSERTG_NOT_NULL(fxt, err);
> +
> + fxt->private = wtst_comp_fxt_private_create();
> + ZUC_ASSERTG_NOT_NULL(fxt->private, err_free);
> + fxt->sockname = NULL;
> +
> + create_logdir();
> +
> + fflush(NULL); /* important. avoid duplication of output */
> + pid = fork();
> + switch (pid) {
> + case -1: /* Error forking */
> + printf("%s:%d: error: Problem with fork: %d\n",
> + __FILE__, __LINE__, errno);
> + ZUC_MARK_FATAL("Error forking");
> + ZUC_ASSERTG_NE(-1, pid, err_free);
> + break;
> + case 0: { /* child */
> + int rc = EXIT_SUCCESS;
> + char **curr = NULL;
> + int argc = 0;
> + char *argv[MAX_ARGS] = {0};
> + pid_t id = getpid();
> + size_t offset = 0;
> + int logfd = -1;
> + char progname[PATH_MAX] = {0};
> + char logname[PATH_MAX] = {0};
> + char sockparam[MAX_SOCK_PARAM_LEN] = {0};
> + char logparam[PATH_MAX] = {0};
> + sigset_t allsigs;
> +
> + strcpy(sockparam, g_sock_param ? g_sock_param : "--socket");
> + strcat(sockparam, "=");
> + offset = strlen(sockparam);
> + populate_sockname(sockparam + offset,
> + sizeof(sockparam) - offset,
> + id);
What about a good old snprintf? Or even asprintf? strcpy and strcat
make me feel nauseous.
> +
> + if (g_basepath[0])
> + snprintf(progname, sizeof(progname), "%s/%s",
> + g_basepath, g_compname);
> + else
> + snprintf(progname, sizeof(progname), "%s", g_compname);
> +
> + /*
> + * Note: the following snprintf's can have (int)id added
> + * with %d to differentiate runs from multiple simultaneous
> + * instances.
> + */
> +
> + snprintf(logname, sizeof(logname), "%s/logs/%s-serverlog.txt",
> + g_abs_basepath,
> + g_log_name ? g_log_name : "test");
> + unlink(logname);
> +
> + snprintf(logparam, sizeof(logparam),
> + "%s=%s/logs/%s-serverlog.txt",
> + g_log_param ? g_log_param : "--log", g_abs_basepath,
> + g_log_name ? g_log_name : "test");
> +
> + /* Redirect stdout and stderr to a file */
> + snprintf(logname, sizeof(logname), "%s/logs/%s-log.txt",
> + g_abs_basepath,
> + g_log_name ? g_log_name : "test");
> + logfd = open(logname, O_RDWR | O_CREAT | O_TRUNC,
> + S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
> + dup2(logfd, fileno(stdout));
> + dup2(logfd, fileno(stderr));
> + fflush(stdout);
> + fflush(stderr);
> + close(logfd);
> + logfd = -1;
You are mirroring the logging setup from the old suite, nice.
> +
> + argv[argc++] = progname;
> + argv[argc++] = sockparam;
> + argv[argc++] = logparam;
> + if (g_extra_params)
> + for (curr = g_extra_params;
> + *curr && (argc < MAX_ARGS); ++curr)
> + argv[argc++] = *curr;
> + argv[argc] = NULL;
> +
> + sigfillset(&allsigs);
> + sigprocmask(SIG_UNBLOCK, &allsigs, NULL);
> +
> + rc = execv(progname, argv);
> +
> + zuc_cleanup();
> + exit(rc);
> + break;
> + }
> + default: { /* parent */
> + fxt->private->pid = pid;
> + populate_sockname(fxt->private->sockname,
> + sizeof(fxt->private->sockname), pid);
> + fxt->sockname = fxt->private->sockname;
> + wait_for_local_socket(fxt->sockname);
> + }
> + }
> +
> + return fxt;
> +
> +err_free:
> + wtst_comp_fxt_destroy(fxt);
> +err:
> + return NULL;
> +}
> +
> +/**
> + * Destroys a test fixture.
> + *
> + * @param fxt pointer to a test fixture to destroy.
> + * It should have previously been created via wtst_comp_fxt_create().
> + * @see wtst_comp_fxt_create()
> + */
> +void
> +wtst_comp_fxt_destroy(struct wtst_comp_fxt *fxt)
> +{
> + if (!fxt)
> + return;
Useless check.
> +
> + wtst_comp_fxt_private_destroy(fxt->private);
Missing inspecting the exit status of the compositor.
> + free(fxt);
> +}
> +
> +/**
> + * Set the environment to be used for determining behavior.
> + * This includes storing values in global variables for later use by the
> + * constructor.
> + *
> + * @param compname the name of the compositor to launch.
> + * @param argv0 the first arg passsed to the program, normally containing
> + * the name used to execute the program, or NULL if no args are passed to
> + * main().
> + * @param path the PATH environment in use at the time the program is
> + * launched.
Right, but this doc does not actually say what it does with all these.
I had to read this function very slowly to see what it does.
First it checks if there is a compositor file in the argv0 path, and if
not, it tries to find it in PATH. This is what the documentation should
also say.
For weston's test suite, using PATH is not right. We never want to
launch an installed weston, as that may be whatever arbitrary version.
The old test suite uses $abs_builddir to be certain to find the correct
files, but relying on that is awkward, because the variable exists only
inside a 'make' invocation. So I think using argv0 dirname is the best
we can do.
> + */
> +void
> +wtst_comp_fxt_set_environment(const char *compname,
> + const char *argv0,
As noted elsewhere, can't you get argv0 by calling
zuc_get_program_name()?
> + const char *path)
> +{
> + char tmp[PATH_MAX] = {0};
> + struct stat st = {.st_size = 0};
> +
> + strncpy(g_compname, compname ? compname : "weston", sizeof(g_compname));
> + g_basepath[0] = 0;
> + free(g_pathenv);
> + g_pathenv = NULL;
> + if (path)
> + g_pathenv = strdup(path);
> +
> + if (argv0) {
> + char *ptr = strrchr(argv0, '/');
> +
> + if (ptr) {
> + size_t len = ptr - argv0;
> +
> + strncpy(tmp, argv0, len);
> + strcat(tmp, "/");
> + strcat(tmp, g_compname);
> + if (!stat(tmp, &st))
> + strncpy(g_basepath, argv0, len);
if (!stat()) is actually a test for file_exists()? I would recommend
writing a function is_executable() for that.
> + }
> + }
> +
> + if (g_pathenv && !g_basepath[0]) {
> + char *save = NULL;
> + char *ptr = strdup(g_pathenv);
> + char *val = strtok_r(ptr, ":", &save);
> + while (val && !g_basepath[0]) {
> + snprintf(tmp, sizeof(tmp), "%s/%s", val, g_compname);
> + if (!stat(tmp, &st))
> + strncpy(g_basepath, val, sizeof(g_basepath));
> + else
> + val = strtok_r(NULL, ":", &save);
> + }
> + free(ptr);
> + }
> +
> + g_abs_basepath = realpath(g_basepath[0] ? g_basepath : ".", NULL);
> +}
> +
> +/**
> + * Returns a copy of the current base path in absolute form.
> + */
> +char *
> +wtst_comp_fxt_get_absbase(void)
> +{
> + return g_abs_basepath ? strdup(g_abs_basepath) : realpath(".", NULL);
> +}
> +
> +/**
> + * Sets parameters to use for launching the compositor.
> + * @param sock_param parameter string to use for setting the socket.
> + * @param log_param parameter string to use for setting the log.
> + * @param params additional parameters to include, or NULL for none.
The additional parameters are good to be settable, but the rest are
just always constants for weston.
You forgot to describe log_name.
> + */
> +void
> +wtst_comp_fxt_set_params(const char *log_name, const char *log_param,
> + const char *sock_param, char **params)
> +{
> + size_t i = 0;
> + size_t count = 0;
> + char **curr = NULL;
> +
> + free(g_sock_param);
> + free(g_log_param);
> + free(g_log_name);
> + g_sock_param = sock_param ? strdup(sock_param) : NULL;
> + g_log_param = log_param ? strdup(log_param) : NULL;
> + g_log_name = log_name ? strdup(log_name) : NULL;
> +
> +
> + if (g_extra_params) {
> + for (curr = g_extra_params; *curr; ++curr) {
> + free(*curr);
> + }
> + free(g_extra_params);
> + g_extra_params = NULL;
> + }
> + if (params) {
> + for (curr = params; *curr; ++curr)
> + count++;
> + g_extra_params = calloc(count + 1, sizeof(char*));
> + for (i = 0; i < count; ++i)
> + g_extra_params[i] = strdup(params[i]);
> + }
> +}
> diff --git a/tools/wayland_fixtures/src/wtst_fixtures.c b/tools/wayland_fixtures/src/wtst_fixtures.c
> new file mode 100644
> index 0000000..91225d5
> --- /dev/null
> +++ b/tools/wayland_fixtures/src/wtst_fixtures.c
> @@ -0,0 +1,1238 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + */
> +
> +#include "config.h"
> +
> +#include "wtst_fixtures.h"
> +
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +#include "shared/helpers.h"
> +#include "shared/os-compatibility.h"
> +#include "shared/zalloc.h"
> +#include "zunitc/zunitc.h"
> +
> +/**
> + * Private data for implementation of test fixture context.
> + */
> +struct wtst_ctx_private {
> +
> + /** List of all known global advertised by the registry. */
> + struct wl_list global_list;
> +
> + /** List of formats supported by the wl_shm. */
> + struct wl_list format_list;
> +
> + /** Chained registry listener to invoke. */
> + struct wl_registry_listener *chained;
> +
> + /** Data to pass back to chained registry listener. */
> + void *chained_data;
> +
> + /** Current main compositor name advertised by the registry. */
> + uint32_t wl_compositor_name;
> +
> + /** Current main compositor version advertised by the registry. */
> + uint32_t wl_compositor_version;
> +
> + /** Current shared memory object name advertised by the registry. */
> + uint32_t wl_shm_name;
> +
> + /** Current shared memory object version advertised by the registry. */
> + uint32_t wl_shm_version;
> +
> + /** Current wl_shell/xdg_shell name advertised by the registry. */
> + uint32_t wl_shell_name;
> +
> + /** Current wl_shell/xdg_shell version advertised by the registry. */
> + uint32_t wl_shell_version;
> +};
> +
> +static void
> +input_update_devices(struct wtst_input *input);
> +
> +static enum wtst_dbgevents_pointer g_pointer_dbg_mask = WTST_DBG_POINTER_ALL;
> +
> +/**
> + * Sets what events will be dumped during testing.
> + *
> + * @param mask a mask of which events to dump out.
> + */
> +void
> +wtst_set_dump_pointer_events(enum wtst_dbgevents_pointer mask)
> +{
> + g_pointer_dbg_mask = mask;
> +}
> +
> +static void
> +input_destroy(struct wtst_input *inp)
> +{
> + wl_list_remove(&inp->link);
> + if (inp->seat_version >= WL_SEAT_RELEASE_SINCE_VERSION)
> + wl_seat_release(inp->wl_seat);
> + else
> + wl_seat_destroy(inp->wl_seat);
> + free(inp);
> +}
> +
> +/**
> + * Private data for implementation of pointer tracking.
> + */
> +struct
> +wtst_pointer_private
> +{
> + /** opaque data. */
> + void *data;
> +
> + /** connected listener. */
> + const struct wl_pointer_listener *listener;
> +
> + /** list implementation hook. */
> + struct wl_list link;
> +};
My previous comments for this still apply:
From the code below, it looks like this should be a list, not
link. Hmm, but you use the same struct as both owning the list
and the items in the list.
Seem like there should be two different structs: the pointer
private, and the item in the list.
It would be good to document for both list and link members
which struct is the list item and, if unique, the list head
owner owner.
So, we should have this instead:
struct wtst_pointer_private {
struct wl_list *listener_list; /* struct wtst_pointer_listener */
};
struct wtst_pointer_listener {
/** opaque data. */
void *data;
/** connected listener. */
const struct wl_pointer_listener *listener;
/** List owned by wtst_pointer_private::listener_list */
struct wl_list link;
};
Also note how the style differs from function definitions.
However, this could all be left out for now, see below.
> +static void
> +pointer_handle_enter(void *data, struct wl_pointer *wl_pointer,
> + uint32_t serial, struct wl_surface *wl_surface,
> + wl_fixed_t x, wl_fixed_t y)
> +{
> + struct wtst_pointer *pointer = data;
> + struct wtst_pointer_private *entry = NULL;
> + struct wtst_pointer_private *tmp = NULL;
> +
> + pointer->focus = wl_surface_get_user_data(wl_surface);
> + pointer->x = wl_fixed_to_int(x);
> + pointer->y = wl_fixed_to_int(y);
> +
> + if (g_pointer_dbg_mask & WTST_DBG_POINTER_ENTER)
> + fprintf(stderr, "test-client: "
> + "got pointer enter %d %d, surface %p\n",
> + pointer->x, pointer->y, pointer->focus);
> +
> + wl_list_for_each_safe(entry, tmp, &pointer->private->link, link)
> + if (entry->listener && entry->listener->enter)
> + entry->listener->enter(entry->data, wl_pointer,
> + serial, wl_surface, x, y);
I think checking for entry->listener and entry->listener->enter should
not be here. I would consider it a test writer's mistake to add a
listener that doesn't have a listener with all function pointers set.
On the other hand, it could be convenient if a test that is only
interested in few events does not have to fill in all hooks with
stubs, but it seems waycheck.c already has stubs for everything.
Btw. this brings up an interesting question about protocol interface
versions. If one implements a new version of an interface in
wtst_fixtures, does that imply that one must update all tests and other
fixtures to manage the new version too?
What about passing the maximum supported protocol interface version as
an argument to the fixture initializer somehow? Assert that it's within
what the fixture itself supports. But that will get awkward as the
number of wrapped interfaces grows.
These comments apply also to all wl_pointer event handlers below.
Only one thing in waycheck calls wtst_pointer_add_listener(), and I
can't see it doing any actual testing. It is hooked up to set a cursor
and check some things, but I cannot see anything actually controlling
the pointer in the compositor and causing the events you expect.
Therefore I'd propose to remove wtst_pointer_add_listener() and support
for it for now to get initial waycheck landed, and come back to it
later.
> +}
> +
> +static void
> +pointer_handle_leave(void *data, struct wl_pointer *wl_pointer,
> + uint32_t serial, struct wl_surface *wl_surface)
> +{
> +static void
> +reg_global_remove(void *data,
> + struct wl_registry *registry,
> + uint32_t name)
> +{
> + struct wtst_ctx *ctx = (struct wtst_ctx *)data;
> +
> + /*
> + * Note that removal of wl_compositor, wl_shm and wl_shell are not
> + * likely to occur. However, handling the cases will make crashes
> + * easier to debug if some implementation has an odd behavior.
> + */
I think we could just abort() the whole test if any of the below get
removed. Their removal is something that literally no-one expects to
handle.
wl_output and wl_seat can get removed, but wl_compositor, wl_shm or
wl_shell never.
> + if (name == ctx->private->wl_compositor_name) {
> + if (ctx->compositor)
> + wl_compositor_destroy(ctx->compositor);
> + ctx->compositor = NULL;
> + ctx->private->wl_compositor_name = 0;
> + ctx->private->wl_compositor_version = 0;
> + } else if (name == ctx->private->wl_shm_name) {
> + if (ctx->shm)
> + wl_shm_destroy(ctx->shm);
> + ctx->shm = NULL;
> + ctx->private->wl_shm_name = 0;
> + ctx->private->wl_shm_version = 0;
> + } else if (name == ctx->private->wl_shell_name) {
> + if (ctx->shell)
> + wl_shell_destroy(ctx->shell);
> + ctx->shell = NULL;
> + ctx->private->wl_shell_name = 0;
> + ctx->private->wl_shell_version = 0;
> + }
> +
> + if (ctx->private->chained && ctx->private->chained->global_remove) {
> + ctx->private->chained->global_remove(ctx->private->chained_data,
> + registry, name);
> + }
> +}
> +/**
> + * Adds a listener to a specified pointer.
> + *
> + * @param pointer the pointer instance to add a listener to.
> + * @param listener the listener to add.
> + * @param data callback data.
> + */
> +int
> +wtst_pointer_add_listener(struct wtst_pointer *pointer,
> + const struct wl_pointer_listener *listener,
> + void *data)
> +{
> + struct wtst_pointer_private *priv = zalloc(sizeof(*priv));
> + ZUC_ASSERTG_NOT_NULL(priv, err);
> + priv->data = data;
> + priv->listener = listener;
> + wl_list_insert(&pointer->private->link, &priv->link);
> + return 0;
> +err:
> + return -1;
> +}
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160225/45f68bd1/attachment-0001.sig>
More information about the wayland-devel
mailing list