Re: [hackers] [sbase][PATCH] Add test framework with a test for tty(1)

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Sun, 8 Jul 2018 12:03:21 +0200

Hi Mattias

Just some comments below (please ignore the mangled formatting.)

On Sat, Jul 7, 2018 at 11:26 PM, Mattias Andrée <maandree_AT_kth.se> wrote:
> Signed-off-by: Mattias Andrée <maandree_AT_kth.se>
> ---
> Makefile | 20 +-
> test-common.c | 823 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> test-common.h | 190 ++++++++++++++
> tty.test.c | 26 ++
> 4 files changed, 1055 insertions(+), 4 deletions(-)
> create mode 100644 test-common.c
> create mode 100644 test-common.h
> create mode 100644 tty.test.c
>
> diff --git a/Makefile b/Makefile
> index 0e421e7..005cf13 100644
> --- a/Makefile
> +++ b/Makefile
> _AT_@ -1,7 +1,7 @@
> include config.mk
>
> .SUFFIXES:
> -.SUFFIXES: .o .c
> +.SUFFIXES: .test .test.o .o .c
>
> HDR =\
> arg.h\
> _AT_@ -19,7 +19,8 @@ HDR =\
> sha512-256.h\
> text.h\
> utf.h\
> - util.h
> + util.h\
> + test-common.h
>
> LIBUTF = libutf.a
> LIBUTFSRC =\
> _AT_@ -181,9 +182,12 @@ BIN =\
> xinstall\
> yes
>
> +TEST =\
> + tty.test
> +
> LIBUTFOBJ = $(LIBUTFSRC:.c=.o)
> LIBUTILOBJ = $(LIBUTILSRC:.c=.o)
> -OBJ = $(BIN:=.o) $(LIBUTFOBJ) $(LIBUTILOBJ)
> +OBJ = $(BIN:=.o) $(TEST:=.o) test-common.o $(LIBUTFOBJ) $(LIBUTILOBJ)
> SRC = $(BIN:=.c)
> MAN = $(BIN:=.1)
>
> _AT_@ -193,12 +197,17 @@ $(BIN): $(LIB) $(@:=.o)
>
> $(OBJ): $(HDR) config.mk
>
> +$(TEST): $(_AT_:=.o) test-common.o
> +
> .o:
> $(CC) $(LDFLAGS) -o $_AT_ $< $(LIB)
>
> .c.o:
> $(CC) $(CFLAGS) $(CPPFLAGS) -o $_AT_ -c $<
>
> +.test.o.test:
> + $(CC) $(LDFLAGS) -o $_AT_ $< test-common.o
> +
> $(LIBUTF): $(LIBUTFOBJ)
> $(AR) rc $_AT_ $?
> $(RANLIB) $_AT_
> _AT_@ -212,6 +221,9 @@ getconf.o: getconf.h
> getconf.h: getconf.sh
> ./getconf.sh > $_AT_
>
> +check: $(TEST) $(BIN)
> + _AT_set -e; for f in $(TEST); do echo ./$$f; ./$$f; done
> +
> install: all
> mkdir -p $(DESTDIR)$(PREFIX)/bin
> cp -f $(BIN) $(DESTDIR)$(PREFIX)/bin
> _AT_@ -271,7 +283,7 @@ sbase-box-uninstall: uninstall
> cd $(DESTDIR)$(PREFIX)/bin && rm -f sbase-box
>
> clean:
> - rm -f $(BIN) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
> + rm -f $(BIN) $(TEST) $(OBJ) $(LIB) sbase-box sbase-$(VERSION).tar.gz
> rm -f getconf.h
>
> .PHONY: all install uninstall dist sbase-box sbase-box-install sbase-box-uninstall clean
> diff --git a/test-common.c b/test-common.c
> new file mode 100644
> index 0000000..458b094
> --- /dev/null
> +++ b/test-common.c
> _AT_@ -0,0 +1,823 @@
> +/* See LICENSE file for copyright and license details. */
> +#include "test-common.h"
> +
> +struct Counter {
> + const char *name;
> + size_t value;
> +};
> +
> +const char *test_file = NULL;
> +int test_line = 0;
> +int main_ret = 0;
> +int timeout = 10;
> +int pdeath_sig = SIGINT;
> +void (*atfork)(void) = NULL;
> +
> +static struct Counter counters[16];
> +static size_t ncounters = 0;
> +static pid_t async_pids[1024];
> +static size_t async_npids = 0;
> +
> +static void
> +eperror(const char *prefix)
> +{
> + perror(prefix);
> + fflush(stderr);
> + exit(64);
> +}
> +
> +struct Process *
> +stdin_text(struct Process *proc, char *s)
> +{
> + proc->input[0].data = s;
> + proc->input[0].flags &= ~IO_STREAM_BINARY;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_bin(struct Process *proc, char *s, size_t n)
> +{
> + proc->input[0].data = s;
> + proc->input[0].len = n;
> + proc->input[0].flags |= IO_STREAM_BINARY;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->input[0].flags &= ~IO_STREAM_DATA;
> + proc->input[0].input_fd = input_fd;
> + proc->input[0].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stdin_type(struct Process *proc, int type)
> +{
> + proc->input[0].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->input[0].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +stdout_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->output[0].flags &= ~IO_STREAM_DATA;
> + proc->output[0].input_fd = input_fd;
> + proc->output[0].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stdout_type(struct Process *proc, int type)
> +{
> + proc->output[0].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->output[0].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +stderr_fds(struct Process *proc, int input_fd, int output_fd)
> +{
> + proc->output[1].flags &= ~IO_STREAM_DATA;
> + proc->output[1].input_fd = input_fd;
> + proc->output[1].output_fd = output_fd;
> + return proc;
> +}
> +
> +struct Process *
> +stderr_type(struct Process *proc, int type)
> +{
> + proc->output[1].flags &= ~IO_STREAM_CREATE_MASK;
> + proc->output[1].flags |= type;
> + return proc;
> +}
> +
> +struct Process *
> +set_preexec(struct Process *proc, void (*preexec)(struct Process *))
> +{
> + proc->preexec = preexec;
> + return proc;
> +}
> +
> +struct Process *
> +set_async(struct Process *proc)
> +{
> + proc->flags |= PROCESS_ASYNC;
> + return proc;
> +}
> +
> +struct Process *
> +set_setsid(struct Process *proc)
> +{
> + proc->flags |= PROCESS_SETSID;
> + return proc;
> +}
> +
> +void
> +push_counter(const char *name)
> +{
> + if (ncounters == ELEMSOF(counters)) {
> + fprintf(stderr, "too many counters\n");
> + fflush(stderr);
> + exit(64);
> + }
> + counters[ncounters++].name = name;
> +}
> +
> +void
> +set_counter(size_t value)
> +{
> + if (!ncounters) {
> + fprintf(stderr, "no counters have been pushed\n");
> + fflush(stderr);
> + exit(64);
> + }
> + counters[ncounters - 1].value = value;
> +}
> +
> +void
> +pop_counter(void)
> +{
> + if (!ncounters) {
> + fprintf(stderr, "all counters have already been popped\n");
> + fflush(stderr);
> + exit(64);
> + }
> + ncounters--;
> +}
> +
> +pid_t
> +async_fork(int *retp)
> +{
> + pid_t pid;
> +
> + if (!retp)
> + retp = &(int){0};
> +
> + if (async_npids == ELEMSOF(async_pids))
> + *retp = async_join();
> +
> + switch ((pid = fork())) {
> + case -1:
> + eperror("fork");
> + case 0:
> + if (atfork)
> + atfork();
> + async_npids = 0;
> +#ifdef PR_SET_PDEATHSIG
> + prctl(PR_SET_PDEATHSIG, pdeath_sig);
> +#endif
> + alarm(timeout);
> + break;
> + default:
> + async_pids[async_npids++] = pid;
> + break;
> + }
> +
> + return pid;
> +}
> +
> +int
> +async_join(void)
> +{
> + int ret = 0, status;
> +
> + while (async_npids--) {
> + if (waitpid(async_pids[async_npids], &status, 0) != async_pids[async_npids])
> + eperror("waitpid");
> + ret |= WIFEXITED(status) ? WEXITSTATUS(status) : 64;
> + }
> +
> + main_ret |= status;
> + async_npids = 0;
> + return ret;
> +}
> +
> +void
> +start_process(struct Process *proc)
> +{
> + struct InputStream *in;
> + struct OutputStream *out;
> + int exec_sig_pipe[2];
> + int fd, minfd = 3, fds[2], status;
> + size_t i;
> + ssize_t r;
> + pid_t pid;
> +
> + if (!proc->file)
> + proc->file = proc->argv[0];
> +
> + for (i = 0; i < proc->ninput; i++)
> + if (minfd <= proc->input[i].fd)
> + minfd = proc->input[i].fd + 1;
> + for (i = 0; i < proc->noutput; i++)
> + if (minfd <= proc->output[i].fd)
> + minfd = proc->output[i].fd + 1;
> +
> + if (pipe(exec_sig_pipe))
> + eperror("pipe");
> +
> + for (i = 0; i < 2; i++) {
> + fd = fcntl(exec_sig_pipe[i], F_DUPFD_CLOEXEC, minfd);
> + if (fd < 0)
> + eperror("fcntl F_DUPFD_CLOEXEC");
> + close(exec_sig_pipe[i]);
> + exec_sig_pipe[i] = fd;
> + }
> +
> + for (i = 0; i < proc->ninput; i++) {
> + in = &proc->input[i];
> + in->name = NULL;
> + if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_PIPE) {
> + if (pipe(fds))
> + eperror("pipe");
> + } else if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_STREAM) {
> + if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_STREAM");
> + } else if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_DGRAM) {
> + if (socketpair(PF_LOCAL, SOCK_DGRAM, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_DGRAM");
> + } else if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_SEQPACKET) {
> + if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_SEQPACKET");
> + } else if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_TTY_NOCTTY) {
> + in->name = strdup(openpt(0, fds));
> + if (!in->name)
> + eperror("strdup");
> + if (!(in->flags & IO_STREAM_MASTER))
> + fd = fds[0], fds[0] = fds[1], fds[1] = fd;
> + } else if ((in->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_TTY_CTTY) {

Instead of repeating

(in->flags & IO_STREAM_CREATE_MASK)

5 times just setting it once like

int createmask = in->flags & IO_STREAM_CREATE_MASK

and then using "createmask == etc." looks cleaner.


> + in->name = strdup(openpt(1, fds));
> + if (!in->name)
> + eperror("strdup");
> + if (!(in->flags & IO_STREAM_MASTER))
> + fd = fds[0], fds[0] = fds[1], fds[1] = fd;
> + } else {
> + goto precreated_input;
> + }
> + in->input_fd = fds[0];
> + in->output_fd = fds[1];
> + precreated_input:
> + if (in->flags & IO_STREAM_DATA) {
> + switch (pid = fork()) {
> + case -1:
> + eperror("fork");
> + case 0:
> + break;
> + default:
> + if (waitpid(pid, &status, 0) != pid)
> + eperror("waitpid");
> + if (status)
> + exit(64);
> + if (!(in->flags & IO_STREAM_KEEP_OPEN))
> + close(in->output_fd);
> + continue;
> + }
> + if (atfork)
> + atfork();
> + dealloc_process(proc);
> + close(in->input_fd);
> + close(exec_sig_pipe[0]);
> + close(exec_sig_pipe[1]);
> + while (i--) {
> + if (proc->input[i].input_fd > 0)
> + close(proc->input[i].input_fd);
> + if (proc->input[i].output_fd > 0)
> + close(proc->input[i].output_fd);
> + }
> + switch (pid = fork()) {
> + case -1:
> + eperror("fork");
> + case 0:
> + exit(0);
> + default:
> + break;
> + }
> + while (in->len) {
> + r = write(in->output_fd, in->data, in->len);
> + if (r < 0)
> + eperror("write");
> + in->data += r;
> + in->len -= (size_t)r;
> + }
> + close(in->output_fd);
> + exit(0);
> + }
> + }
> +
> + for (i = 0; i < proc->noutput; i++) {
> + out = &proc->output[i];
> + out->name = NULL;
> + if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_PIPE) {
> + if (pipe(fds))
> + eperror("pipe");
> + } else if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_STREAM) {
> + if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_STREAM");
> + } else if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_DGRAM) {
> + if (socketpair(PF_LOCAL, SOCK_DGRAM, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_DGRAM");
> + } else if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_SOCK_SEQPACKET) {
> + if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, fds))
> + eperror("socketpair PF_LOCAL SOCK_SEQPACKET");
> + } else if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_TTY_NOCTTY) {
> + out->name = strdup(openpt(0, fds));
> + if (!out->name)
> + eperror("strdup");
> + if (out->flags & IO_STREAM_MASTER)
> + fd = fds[0], fds[0] = fds[1], fds[1] = fd;
> + } else if ((out->flags & IO_STREAM_CREATE_MASK) == IO_STREAM_TTY_CTTY) {
> + out->name = strdup(openpt(1, fds));
> + if (!out->name)
> + eperror("strdup");
> + if (out->flags & IO_STREAM_MASTER)
> + fd = fds[0], fds[0] = fds[1], fds[1] = fd;
> + } else {
> + continue;
> + }
> + out->input_fd = fds[0];
> + out->output_fd = fds[1];
> + }
> +
> + switch ((proc->pid = fork())) {
> + case -1:
> + eperror("fork");
> + case 0:
> + break;
> + default:
> + if (atfork)
> + atfork();
> + for (i = 0; i < proc->ninput; i++)
> + close(proc->input[i].input_fd);
> + for (i = 0; i < proc->noutput; i++)
> + close(proc->output[i].output_fd);
> + close(exec_sig_pipe[1]);
> + read(exec_sig_pipe[0], &(char){0}, 1);
> + if (clock_gettime(CLOCK_MONOTONIC, &proc->start_time))
> + eperror("clock_gettime CLOCK_MONOTONIC");
> + close(exec_sig_pipe[0]);
> + return;
> + }
> +
> +#ifdef PR_SET_PDEATHSIG
> + prctl(PR_SET_PDEATHSIG, proc->pdeath_sig);
> +#endif
> +
> + for (i = 0; i < proc->ninput; i++) {
> + fd = fcntl(proc->input[i].input_fd, F_DUPFD, minfd);
> + if (fd < 0)
> + eperror("fcntl F_DUPFD");

Would be nice to know if it failed on the input or output side:

eperror("input fcntl F_DUPFD");

and

eperror("output fcntl F_DUPFD");

respectively.

> + close(proc->input[i].input_fd);
> + proc->input[i].input_fd = fd;
> + }
> +
> + for (i = 0; i < proc->noutput; i++) {
> + fd = fcntl(proc->output[i].output_fd, F_DUPFD, minfd);
> + if (fd < 0)
> + eperror("fcntl F_DUPFD");

See above.

> + close(proc->output[i].output_fd);
> + proc->output[i].output_fd = fd;
> + }
> +
> + for (i = 0; i < proc->ninput; i++) {
> + if (dup2(proc->input[i].input_fd, proc->input[i].fd) != proc->input[i].fd)
> + eperror("dup2");

See above.


> + close(proc->input[i].input_fd);
> + }
> +
> + for (i = 0; i < proc->noutput; i++) {
> + if (dup2(proc->output[i].output_fd, proc->output[i].fd) != proc->output[i].fd)
> + eperror("dup2");

See above.

I will have a closer look at everything after you have slimmed down the
test-common.{c,h} (as discussed).


Cheers,

Silvan
Received on Sun Jul 08 2018 - 12:03:21 CEST