2
\$\begingroup\$

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)?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ There's a lot of "how" here, but no what or why. What is the actual operation you're blocking on? \$\endgroup\$
    – Reinderien
    Jun 23, 2023 at 22:27
  • \$\begingroup\$ @Reinderien it blocks on poll(2) on a file descriptor for the socket for the connection to the X server. The actual program is a GUI file manager which can spawn a script on user interaction (this script can, for example open a xmessage dialog asking whether to delete file when the user press DEL, or popup a xmenu when the user right clicks a file). It needs to wait for this script to terminate while kkeep responding to window events (like resizing or closing the window) from the X connection. \$\endgroup\$
    – Seninha
    Jun 23, 2023 at 23:15
  • \$\begingroup\$ I think that what is blocking on does not quite matter. My intention with this snippet is to wait for a child process while blocking on the read of a file descriptor. What I want to know is whether that approach is good (the approach being a thread waiting for the child before writing into one end of a self-pipe; while the main thread is polling both the reading end of the pipe and the file descriptor I'm interested in). \$\endgroup\$
    – Seninha
    Jun 23, 2023 at 23:19

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Hi, thanks for the answer! I ended up using another solution on my program, it is still based on a self-pipe, but it uses the closing of the write end of the pipe rather than writing a byte into it. On handling errors on close(2) I'm now checking for errors, especially EINTR. \$\endgroup\$
    – Seninha
    Jun 24, 2023 at 20:53
  • \$\begingroup\$ Sure, using the closing of the pipe instead of sending a byte will also work, but I don't think that makes any difference in portability or performance. About checking for EINTR: be aware that it's unspecified if the filedescriptor is actually already closed when you receive EINTR (like on Linux), or that it's still open and that you have to try to call close() again (HP-UX does this apparently). So if you do try again, then if you get EBADF the second time, that should not count as an error. \$\endgroup\$
    – G. Sliepen
    Jun 24, 2023 at 21:38
2
\$\begingroup\$

"%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]);
\$\endgroup\$
2
  • \$\begingroup\$ But isn't it well-defined in C99? \$\endgroup\$
    – G. Sliepen
    Jun 24, 2023 at 21:44
  • 1
    \$\begingroup\$ @G.Sliepen ssize_t is not part of the standard C library nor C proper. It is not defined in C99. The corresponding signed integer type of size_t, mentioned in C99, and *nix ssize_t do not share the same definition. They might have the same size/range - might not. In particular ssize_t is sometimes 2x the width of size_t to fully encompass the size_t range. \$\endgroup\$ Jun 24, 2023 at 22:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.