patterncMinor
Function to grab stdin, stdout, stderr of a child process
Viewed 0 times
stdoutstderrgrabprocessstdinfunctionchild
Problem
I've managed to create a function to pipe the
A cleaning function for it:
``
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;
}
`Solution
There might be good use cases for
but this is not one of them.
It's best when code reads from top to bottom.
The
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
the code becomes:
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
and again later in the code if
closing
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
for example:
Avoid comments when possible.
Instead of a comment,
try to code in a way to make the comment unnecessary.
For example:
This comment could be replaced with this statement:
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
And we're kind of back to the pesky
Without the
this concern here about the comment and the logic wouldn't exist in the first place.
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.
Code Snippets
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;
}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;/* This section is never be reached without a goto */return pid;Context
StackExchange Code Review Q#71793, answer score: 4
Revisions (0)
No revisions yet.