6
\$\begingroup\$

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?

\$\endgroup\$
1
4
\$\begingroup\$

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.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Amr Ayman
    Dec 6 '14 at 8:17
  • \$\begingroup\$ 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? \$\endgroup\$
    – janos
    Dec 6 '14 at 10:10
  • \$\begingroup\$ @AmrAyman I improved my explanation against goto, and added an improvement idea with goto. \$\endgroup\$
    – janos
    Dec 6 '14 at 11:20
  • \$\begingroup\$ Well, I get your point, but (noting that this is a really simple example), on complicated stuff, duplication would be a real disaster. \$\endgroup\$
    – Amr Ayman
    Dec 6 '14 at 12:09
  • 1
    \$\begingroup\$ The linux kernel uses this technique all the time. \$\endgroup\$
    – Amr Ayman
    Dec 6 '14 at 12:38
3
\$\begingroup\$

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

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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