Having trouble with fork(), pipe(), dup2() and exec() in C
Here's my code:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <wait.h>
#include <readline/readline.h>
#define NUMPIPES 2
int main(int argc, char *argv[]) {
char *bBuffer, *sPtr, *aPtr = NULL, *pipeComms[NUMPIPES], *cmdArgs[10];
int fdPipe[2], pCount, aCount, i, status, lPids[NUMPIPES];
pid_t pid;
pipe(fdPipe);
while(1) {
bBuffer = readline("Shell> ");
if(!strcasecmp(bBuffer, "exit")) {
return 0;
}
sPtr = bBuffer;
pCount = -1;
do {
aPtr = strsep(&sPtr, "|");
pipeComms[++pCount] = aPtr;
} while(aPtr);
for(i = 0; i < pCount; i++) {
aCount = -1;
do {
aPtr = strsep(&pipeComms[i], " ");
cmdArgs[++aCount] = aPtr;
} while(aPtr);
cmdArgs[aCount] = 0;
if(strlen(cmdArgs[0]) > 0) {
pid = fork();
if(pid == 0) {
if(i == 0) {
close(fdPipe[0]);
dup2(fdPipe[1], STDOUT_FILENO);
close(fdPipe[1]);
} else if(i == 1) {
close(fdPipe[1]);
dup2(fdPipe[0], STDIN_FILENO);
close(fdPipe[0]);
}
execvp(cmdArgs[0], cmdArgs);
exit(1);
} else {
lPids[i] = pid;
/*waitpid(pid, &status, 0);
if(WIFEXITED(status)) {
printf("[%d] TERMINATED (Status: %d)\n",
pid, WEXITSTATUS(status));
}*/
}
}
}
for(i = 0; i < pCount; i++) {
waitpid(lPids[i], &status, 0);
if(WIFEXITED(status)) {
printf("[%d] TERMINATED (Status: %d)\n",
lPids[i], WEXITSTATUS(status));
}
}
}
return 0;
}
(The code was updated to reflect he changes proposed by two answers below, it still doesn't work as it should...)
Here's the test case where this fails:
nazgulled ~/Projects/SO/G08 $ ls -l
total 8
-rwxr-xr-x 1 nazgulled nazgulled 7181 2009-05-27 17:44 a.out
-rwxr-xr-x 1 nazgulled nazgulled 754 2009-05-27 01:42 data.h
-rwxr-xr-x 1 nazgulled nazgulled 1305 2009-05-27 17:50 main.c
-rwxr-xr-x 1 nazgulled nazgulled 320 2009-05-27 01:42 makefile
-rwxr-xr-x 1 nazgulled nazgulled 14408 2009-05-27 17:21 prog
-rwxr-xr-x 1 nazgulled nazgulled 9276 2009-05-27 17:21 prog.c
-rwxr-xr-x 1 nazgulled nazgulled 10496 2009-05-27 17:21 prog.o
-rwxr-xr-x 1 nazgulled nazgulled 16 2009-05-27 17:19 test
nazgulled ~/Projects/SO/G08 $ ./a.out
Shell> ls -l|grep prog
[4804] TERMINATED (Status: 0)
-rwxr-xr-x 1 nazgulled nazgulled 14408 2009-05-27 17:21 prog
-rwxr-xr-x 1 nazgulled nazgulled 9276 2009-05-27 17:21 prog.c
-rwxr-xr-x 1 nazgulled nazgulled 10496 2009-05-27 17:21 prog.o
The problem is that I should return to my shell after that, I should see "Shell> " waiting for more input. You can also notice that you don't see a message similar to "[4804] TERMINATED (Status: 0)" (but with a different pid), which means the second process didn't terminate.
I think it has something to do with grep, because this works:
nazgulled ~/Projects/SO/G08 $ ./a.out
Shell> echo q|sudo fdisk /dev/sda
[4838] TERMINATED (Status: 0)
The number of cylinders for this disk is set to 1305.
There is nothing wrong with that, but this is larger than 1024,
and could in certain setups cause problems with:
1) software that runs at boot time (e.g., old versions of LILO)
2) booting and partitioning software from other OSs
(e.g., DOS FDISK, OS/2 FDISK)
Command (m for help):
[4839] TERMINATED (Status: 0)
You can easily see two "terminate" messages...
So, what's wrong with my code?
Answer
Even after the first command of your pipeline exits (and thust closes stdout=~fdPipe[1]), the parent still has fdPipe[1] open.
Thus, the second command of the pipeline has a stdin=~fdPipe[0] that never gets an EOF, because the other endpoint of the pipe is still open.
You need to create a new pipe(fdPipe) for each |, and make sure to close both endpoints in the parent; i.e.
for cmd in cmds
if there is a next cmd
pipe(new_fds)
fork
if child
if there is a previous cmd
dup2(old_fds[0], 0)
close(old_fds[0])
close(old_fds[1])
if there is a next cmd
close(new_fds[0])
dup2(new_fds[1], 1)
close(new_fds[1])
exec cmd || die
else
if there is a previous cmd
close(old_fds[0])
close(old_fds[1])
if there is a next cmd
old_fds = new_fds
if there are multiple cmds
close(old_fds[0])
close(old_fds[1])
Also, to be safer, you should handle the case of fdPipe and {STDIN_FILENO,STDOUT_FILENO} overlapping before performing any of the close and dup2 operations. This may happen if somebody has managed to start your shell with stdin or stdout closed, and will result in great confusion with the code here.
Edit
fdPipe1 fdPipe3
v v
cmd1 | cmd2 | cmd3 | cmd4 | cmd5
^ ^
fdPipe2 fdPipe4
In addition to making sure you close the pipe's endpoints in the parent, I was trying to make the point that fdPipe1, fdPipe2, etc. cannot be the same pipe().
/* suppose stdin and stdout have been closed...
* for example, if your program was started with "./a.out <&- >&-" */
close(0), close(1);
/* then the result you get back from pipe() is {0, 1} or {1, 0}, since
* fd numbers are always allocated from the lowest available */
pipe(fdPipe);
close(0);
dup2(fdPipe[0], 0);
I know you don't use close(0) in your present code, but the last paragraph is warning you to watch out for this case.
Edit
The following minimal change to your code makes it work in the specific failing case you mentioned:
@@ -12,6 +12,4 @@
pid_t pid;
- pipe(fdPipe);
-
while(1) {
bBuffer = readline("Shell> ");
@@ -29,4 +27,6 @@
} while(aPtr);
+ pipe(fdPipe);
+
for(i = 0; i < pCount; i++) {
aCount = -1;
@@ -72,4 +72,7 @@
}
+ close(fdPipe[0]);
+ close(fdPipe[1]);
+
for(i = 0; i < pCount; i++) {
waitpid(lPids[i], &status, 0);
This won't work for more than one command in the pipeline; for that, you'd need something like this: (untested, as you have to fix other things as well)
@@ -9,9 +9,7 @@
int main(int argc, char *argv[]) {
char *bBuffer, *sPtr, *aPtr = NULL, *pipeComms[NUMPIPES], *cmdArgs[10];
- int fdPipe[2], pCount, aCount, i, status, lPids[NUMPIPES];
+ int fdPipe[2], fdPipe2[2], pCount, aCount, i, status, lPids[NUMPIPES];
pid_t pid;
- pipe(fdPipe);
-
while(1) {
bBuffer = readline("Shell> ");
@@ -32,4 +30,7 @@
aCount = -1;
+ if (i + 1 < pCount)
+ pipe(fdPipe2);
+
do {
aPtr = strsep(&pipeComms[i], " ");
@@ -43,11 +44,12 @@
if(pid == 0) {
- if(i == 0) {
- close(fdPipe[0]);
+ if(i + 1 < pCount) {
+ close(fdPipe2[0]);
- dup2(fdPipe[1], STDOUT_FILENO);
+ dup2(fdPipe2[1], STDOUT_FILENO);
- close(fdPipe[1]);
- } else if(i == 1) {
+ close(fdPipe2[1]);
+ }
+ if(i != 0) {
close(fdPipe[1]);
@@ -70,4 +72,17 @@
}
}
+
+ if (i != 0) {
+ close(fdPipe[0]);
+ close(fdPipe[1]);
+ }
+
+ fdPipe[0] = fdPipe2[0];
+ fdPipe[1] = fdPipe2[1];
+ }
+
+ if (pCount) {
+ close(fdPipe[0]);
+ close(fdPipe[1]);
}