Wait for child while polling file descriptor with self-pipe controlled by different threads

Question

I am working on a personal project, and one of the things it does is to call a blocking operation on a file descriptor while a forked+exec'ed child process is running. It needs to know when the forked process exits while reading events from that file descriptor.

After some discussion on libera.chat's #posix channel, what I ended up doing was to open a self-pipe and create a new thread. This new thread waits for the child process and writes a byte into the pipe when the child terminates. The main thread poll(2)s both the event file descriptor and the reading end of the pipe in a loop; it will break the loop after getting a single byte from the pipe and do whatever it needs to do when getting something from the event file descriptor. You can check the actual code here.

I am using poll2(2) in order to close both pipe ends after exec'ing on the child.

I abstracted this bit out the actual program into the following snippet, which is what I want to be reviewed:

#include <sys/wait.h>

#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <poll.h>
#include <pthread.h>
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>

#define LEN(a)  (sizeof(a)/sizeof((a)[0]))

struct Child {
    pid_t   pid;
    int     fd;
};

static void *
waitthread(void *arg)
{
    struct Child *child = (struct Child *)arg;
    char byte = 0xFF;

    while (waitpid(child->pid, NULL, 0) == -1)
        if (errno != EINTR)
            err(EXIT_FAILURE, "wait");
    while (write(child->fd, &byte, 1) == -1)
        if (errno != EAGAIN)
            err(EXIT_FAILURE, "write");
    pthread_exit(0);
}

int
main(int argc, char *argv[])
{
    enum { FILE_STDIN, FILE_CHILD };
    enum { FILE_READ, FILE_WRITE };
    struct Child child;
    struct pollfd pollfds[2];
    ssize_t nread;
    pthread_t thrd;
    int pipefds[2];
    char byte;
    char buf[BUFSIZ];

    if (argc < 2) {
        (void)fprintf(stderr, "usage: %s command [argument ...]\n", argv[0]);
        exit(EXIT_FAILURE);
    }
    if (pipe2(pipefds, O_NONBLOCK | O_CLOEXEC) == -1)
        err(EXIT_FAILURE, "pipe2");
    child.fd = pipefds[FILE_WRITE];
    if ((child.pid = fork()) == -1)
        err(EXIT_FAILURE, "fork");
    if (child.pid == 0) {
        execvp(argv[1], argv + 1);
        err(EXIT_FAILURE, "execvp");
    }
    pollfds[FILE_CHILD].fd = pipefds[FILE_READ];
    pollfds[FILE_STDIN].fd = STDIN_FILENO;
    pollfds[FILE_STDIN].events = pollfds[FILE_CHILD].events = POLLIN;
    if (pthread_create(&thrd, NULL, waitthread, &child) != 0)
        errx(EXIT_FAILURE, "could not create thread");
    for (;;) {
        if (poll(pollfds, LEN(pollfds), -1) == -1) {
            if (errno == EINTR)
                continue;
            err(EXIT_FAILURE, "poll");
        }
        if (pollfds[FILE_STDIN].revents & (POLLERR | POLLNVAL))
            errx(EXIT_FAILURE, "%d: bad fd", pollfds[FILE_STDIN].fd);
        if (pollfds[FILE_CHILD].revents & (POLLERR | POLLNVAL))
            errx(EXIT_FAILURE, "%d: bad fd", pollfds[FILE_CHILD].fd);
        if (pollfds[FILE_STDIN].revents & POLLHUP)
            pollfds[FILE_STDIN].fd = -1;
        if (pollfds[FILE_CHILD].revents & POLLHUP)
            pollfds[FILE_CHILD].fd = -1;
        if (pollfds[FILE_CHILD].revents & POLLIN) {
            if (read(pollfds[FILE_CHILD].fd, &byte, 1) != -1) {
                warnx("we are done now");
                break;
            }
            if (errno != EAGAIN)
                err(EXIT_FAILURE, "read");
        }
        if (pollfds[FILE_STDIN].revents & POLLIN) {
            nread = read(pollfds[FILE_STDIN].fd, buf, sizeof(buf));
            if (nread == -1) {
                if (errno == EINTR)
                    continue;
                err(EXIT_FAILURE, "read");
            }
            (void)fprintf(
                stderr,
                "read %zd byte%s from stdin!\n",
                nread,
                &"s"[nread == 1]
            );
        }
    }
    (void)pthread_join(thrd, NULL);
    (void)close(pipefds[FILE_READ]);
    (void)close(pipefds[FILE_WRITE]);
    return EXIT_SUCCESS;
}

Compile it with cc -lpthread c.c and run it passing a command to be spawned, especially one that takes some time to return, like ./a.out sleep 5. It will read what you write into standard input and write into standard output how many bytes it has read; and while doing that, it will also wait for the spawned command to terminate.

Is it OK?
Can it fail somehow?
How can I improve it (without adding any Linuxism)?

Answer 1

Alternative solutions

The self-pipe trick is not a bad one, it can be applied to many situations where you have a the need to poll filedescriptors on one hand, and have to react to events coming from other sources on the other hand. There is virtually nothing that can go wrong with a pipe that you'll only ever send one byte over.

If you could add Linuxisms, I would strongly recommend using signalfd() to get a filedescriptor that you can use to poll for SIGCHLD, which is sent when a child process terminates.

You don't need signalfd() though, you can instead install a signal handler for SIGCHLD which in turn will send a byte over the self-pipe. This avoids having to create and destroy a thread.

Finally, consider that the child process has a stdin and stdout. By default, it's just a copy of the parent's file descriptors. But before calling execve(), you can close stdin and/or stdout in the child and use dup2() to assign other fds to stdin and/or stdout. For example, instead of using the self-pipe for sending a byte when the child process terminates, you can reassign the sending side of the self-pipe to the child's stdout, or reassign the reading side to the child's stdin. Even if you are not interested in sending anything to and receiving anything from the child, you can still detect when the child closes its side of the pipe when it terminates! You probably want to do this with the child's stdin; if you do it with stdout and the child actually writes a lot to it, but you don't read it in the parent, the pipe's internal buffer will at some point become full and writes to it will block the child.

Use return instead of pthread_exit()

You can just return from a thread function, there is no need to call pthread_exit(). Also, the latter takes a void *, so passing it 0 is not correct.

When to ignore return values

I see you added a lot of (void) casts, maybe to silence warnings about unused return values? While this is fine when you fprintf(stderr, …) right before you call exit(EXIT_FAILURE), as you can't do much about that printing failing, consider the last three statements before the return in main():

(void)pthread_join(thrd, NULL);
(void)close(pipefds[FILE_READ]);
(void)close(pipefds[FILE_WRITE]);

If any of these functions would return an error, something bad happened. I'm not sure you can do much about it, but I would print an error message to stderr, and definitely exit with EXIT_FAILURE instead of EXIT_SUCCESS.

Standards compliance

You did not want to use Linuxisms, but you are using a Linux&BSDism: err() and friends are neither C nor POSIX.

Just FYI: while pthreads are obviously POSIX, since C11 there is optional support in C itself for threads, see thrd_create(). It's not a mandatory part of the standard though, and probably POSIX threads are more widely supported.

Answer 2

"%zd" is not specified as matching ssize_t

"%zd" is reasonable yet risks problems.

Although z works for ssize_t on most implementations, portable POSIX programs should avoid using it—for example, by converting the value to intmax_t and using its length modifier (j).

ssize_t nread;
...
// (void)fprintf(stderr, "read %zd byte%s from stdin!\n", nread, &"s"[nread == 1]);
(void)fprintf(stderr, "read %jd byte%s from stdin!\n", (intmax_t) nread, &"s"[nread == 1]);