You do not close enough file file descriptors.
Rule of thumb:
- If you use
dup() or dup2() to duplicate a channel file descriptor for standard input or standard output, you must close both descriptors of the source file of the file.
You should also be sure that if the parent shell creates a pipe, it closes both copies of its file descriptors.
Also note that processes in the pipeline must be started at the same time. In particular, the pipes have a limited capacity, and the process is blocked when there is no space in the pipe. The limit can be quite small (POSIX requires it to be at least 4 KiB, but that's it). If your programs deal with megabytes of data, they must be allowed to run simultaneously in the pipeline. Therefore, waitpid() must occur outside the loop that starts the children. You also need to close the pipes in the parent process before waiting; otherwise, the child reading the pipe will never see the EOF (because the parent could, theoretically, write to the pipe, although it would not).
You have structure members whose names begin with an underscore. This is dangerous. Names starting with an underscore are reserved for implementation. The C standard says:
ISO / IEC 9899: 2011 §7.1.3 Reserved identifiers
- all identifiers starting with an underscore, and either an uppercase letter or another Underline is always reserved for any use.
- All identifiers starting with an underscore are always reserved for use as identifiers with file size in both regular and tag names.
This means that if you encounter problems, then the problem will be yours, not the system. Obviously your code works, but you should be aware of the problems you might encounter, and it would be wiser to avoid them.
Code example
This is a fixed SSCCE based on the code above:
#include <assert.h> #include <stdio.h> #include <stdlib.h> #include <sys/wait.h> #include <unistd.h> typedef struct Command Command; struct Command { int _fd_out; int _fd_in; int _flag_pipe_in; int _flag_pipe_out; char **_commands; }; typedef int Pipe[2]; enum { STDIN = STDIN_FILENO, STDOUT = STDOUT_FILENO, STDERR = STDERR_FILENO }; int main(void) { char *ls_cmd[] = { "ls", 0 }; char *grep_cmd[] = { "grep", "r", 0 }; Command commands[] = { { ._fd_in = 0, ._flag_pipe_in = 0, ._fd_out = 1, ._flag_pipe_out = 1, ._commands = ls_cmd, }, { ._fd_in = 0, ._flag_pipe_in = 1, ._fd_out = 1, ._flag_pipe_out = 0, ._commands = grep_cmd, } }; int commands_num = sizeof(commands) / sizeof(commands[0]); /* Allow valgrind to check memory */ Command *pcommands = malloc(commands_num * sizeof(Command)); for (int i = 0; i < commands_num; i++) pcommands[i] = commands[i]; for (int i = 0; i < commands_num; i++) { //exec all the commands instants if (pcommands[i]._flag_pipe_out == 1) { //creates pipe if necessary Pipe pipe_fd; if (pipe(pipe_fd) == -1) { perror("Error: \"pipe()\" failed"); } pcommands[i]._fd_out = pipe_fd[1]; pcommands[i+1]._fd_in = pipe_fd[0]; } pid_t pid = fork(); //the child exec the commands if (pid == -1) { perror("Error: \"fork()\" failed"); break; } else if (!pid) { //child process if (pcommands[i]._flag_pipe_in == 1) { //if there was a pipe to this command assert(i > 0); assert(pcommands[i-1]._flag_pipe_out == 1); assert(pcommands[i-1]._fd_out > STDERR); if (dup2(pcommands[i]._fd_in, STDIN) == -1) { perror("Error: \"dup2()\" failed"); exit(0); } close(pcommands[i]._fd_in); close(pcommands[i-1]._fd_out); } if (pcommands[i]._flag_pipe_out == 1) { //if there was a pipe from this command assert(i < commands_num - 1); assert(pcommands[i+1]._flag_pipe_in == 1); assert(pcommands[i+1]._fd_in > STDERR); if (dup2(pcommands[i]._fd_out, STDOUT) == -1) { perror("Error: \"dup2()\" failed"); exit(0); } close(pcommands[i]._fd_out); close(pcommands[i+1]._fd_in); } execvp(pcommands[i]._commands[0] , pcommands[i]._commands); //run the command perror("Error: \"execvp()\" failed"); exit(1); } else printf("Child PID %d running\n", (int)pid); } //closing all the open pipe fd's for (int i = 0; i < commands_num; i++) { if (pcommands[i]._fd_in != STDIN) { //if there was another stdin that is not 0 close(pcommands[i]._fd_in); } if (pcommands[i]._fd_out != STDOUT) { //if there was another stdout that is not 1 close(pcommands[i]._fd_out); } } int status; pid_t corpse; while ((corpse = waitpid(-1, &status, 0)) > 0) printf("Child PID %d died with status 0x%.4X\n", (int)corpse, status); free(pcommands); return(0); }
Just for my knowledge, how would you do this, so it won’t get “undeniably dirty”?
I would probably save the channel information so I don’t have to worry about the conventions contained in the statements (access to child information for the child before or after it in the pipeline). If every child only needs access to information in his own data structure, he is cleaner. I would reorganize the "struct Command", so it contained two pipes, as well as indicators for which the pipe contains information that needs to be closed. In many ways, not much different from what you have; just pcommands[i] in this kid, I only need to see pcommands[i] .
You can see the partial answer in a different context in C Minishell adding pipelines .