Function to grab stdin, stdout, stderr of a child process

Question

I've managed to create a function to pipe the stdin, stdout and stderr of a child process and provide them as file descriptors to read and write to:

pid_t opencmd(pipes, path, argv)
 int *const pipes;
 const char *const path;
 char *const *const argv;
{
 if (!pipes || !path || !argv)
 goto error_in;
 int in[2];
 int out[2];
 int err[2];
 pid_t pid;
 /*
 * Creating pipes in between ...
 */
 if (pipe(in))
 goto error_in;
 if (pipe(out))
 goto error_out;
 if (pipe(err))
 goto error_err;
 /*
 * Starting actual processing ...
 */
 pid = fork();
 switch (pid) {
 case -1: /* Error */
 goto error_fork;
 case 0: /* Child */
 close(in[1]);
 close(out[0]);
 close(err[0]);
 dup2(in[0], 0); /* redirect child stdin to in[0] */
 dup2(out[1], 1); /* redirect child stdout to out[1] */
 dup2(err[1], 2); /* redirect child stderr to err[1] */
 execv(path, argv); /* actual command execution */
 return -1; /* shall never be executed */
 default: /* Parent */
 close(in[0]); /* no need to read its stdin */
 close(out[1]); /* no need to write to its stdout */
 close(err[1]); /* no need to write to its stderr */
 pipes[0] = in[1]; /* Write to child stdin */
 pipes[1] = out[0]; /* Read child stdout */
 pipes[2] = err[0]; /* Read child stderr */
 return pid;
 }
/* This section is never be reached without a goto */
 error_fork:
 close(err[0]);
 close(err[1]);
 error_err:
 close(out[0]);
 close(out[1]);
 error_out:
 close(in[0]);
 close(in[1]);
 error_in:
 return -1;
}

A cleaning function for it:

int closecmd(pid, pipes)
 const pid_t pid;
 int *pipes;
{
 int status;
 close(pipes[0]);
 close(pipes[1]);
 close(pipes[2]);
 waitpid(pid, &status, 0);
 return status;
}

Are there any issues I'm overlooking with them?

• Ciro Santilli OurBigBook.com: related stackoverflow.com/questions/9405985/…

Answer 1

There might be good use cases for goto, but this is not one of them.

It's best when code reads from top to bottom. The goto statements break that readable flow. Since the jump-to labels are quite far, I'm forced to scroll to the end to see what they do. After I scrolled, I find some code using variables defined at the beginning, which I haven't memorized earlier, so now I have to go back to top. Effectively I'm making several jumps back and forth to understand how this works, and then a few more jumps to verify carefully that it should work well.

This is a nightmare, and it doesn't have to be. If you replace the goto statements with the code at their jump-to labels, the code becomes:

if (pipe(in)) {
 return -1;
}
if (pipe(out)) {
 close(in[0]);
 close(in[1]);
 return -1;
}
if (pipe(err)) {
 close(out[0]);
 close(out[1]);
 close(in[0]);
 close(in[1]);
 return -1;
}

This is a lot more readable: everything I need to understand these guard statements are right there above.

You probably don't like the duplication when closing in, and again later in the code if fork failed, closing in and out. It's true that code duplication is not good, but I think it's the lesser of evils here, and it's outweighed by the improvement in readability.

If you still disagree, and consider avoiding the duplication more important, then you can improve this technique with goto by using more descriptive labels, for example:

 close_err_and_out_and_in_and_return_with_failure:
 close(err[0]);
 close(err[1]);
 close_out_and_in_and_return_with_failure:
 close(out[0]);
 close(out[1]);
 close_in_and_return_with_failure:
 close(in[0]);
 close(in[1]);
 return_with_failure:
 return -1;

---

Avoid comments when possible. Instead of a comment, try to code in a way to make the comment unnecessary. For example:

/* This section is never be reached without a goto */

This comment could be replaced with this statement:

return pid;

Not only this is short and sweet, it has the great benefit that since it's code, it is enforced: the lines below it will not be reached, no matter what changes in the code above the return statement.

And we're kind of back to the pesky goto statements. Without the goto statements, this concern here about the comment and the logic wouldn't exist in the first place.

• Amr Ayman: I don't favor using gotos but if you look closely, you'll see that using goto (in this case) actually adds to the readability of the code. See the code without gotos. And have a look on examples of good gotos.
• janos: I did consider the version without goto statements before posting my answer. I think it's much more readable and overall better that way. Why do you think the use of goto is worth sacrificing readability?
• janos: @AmrAyman I improved my explanation against goto, and added an improvement idea with goto.
• Amr Ayman: Well, I get your point, but (noting that this is a really simple example), on complicated stuff, duplication would be a real disaster.
• Amr Ayman: The linux kernel uses this technique all the time.

Answer 2

Order of a cleanup is wrong. You must waitpid before closing pipes. Otherwise the child process may be hit by an unexpected SIGPIPE.