exec: Create wrapper to capture stderr output separately from stdout#444
exec: Create wrapper to capture stderr output separately from stdout#444
Conversation
src/exec.cc
Outdated
| stderr_pipe[1] = -1; | ||
|
|
||
| // Read stdout and stderr | ||
| result.stdout_output = readFromPipe(stdout_pipe[0]); |
There was a problem hiding this comment.
I think we can have potential deadlock here if a child process fills up the stderr pipe and blocks while the parent is blocked at reading stdout. I suppose we need to use select or something else to avoid this potential deadlock.
There was a problem hiding this comment.
you can. Something like this is really hard to get right. There's going to be some sig handler or fcntl on a pipe or something we'll miss and will only happen once in a thousand times.
If we can't use an existing API from boost, I'd seriously think about doing something low tech where you use the spawn function but send stderr and stdout to different files.
There was a problem hiding this comment.
I am pretty sure we can avoid deadlock by using non-blocking select/epoll call.
There was a problem hiding this comment.
you can. then you get into learning the subtleties of handling calls to those functions correctly. Its just hard to know if we have this foolproof before a release.
There was a problem hiding this comment.
I've changed the code to use select to check both pipes at the same time.
I can give Boost Process V2 API a try, if that sounds better.
There was a problem hiding this comment.
. Its just hard to know if we have this foolproof before a release.
That's true for any software, regardless whether we use low-level system/OS calls or higher-level libraries like boost (especially boost :)). Only decent testing can help here.
detsch
force-pushed
the
detsch-popen-with-stderr
branch
from
21cf92a to
b945afc
Compare
|
@detsch I've tested on qemu and it works fine for me so far. I suggest to test it on a few arm devices in our lab if it is feasible. |
src/exec.cc
Outdated
| }; | ||
|
|
||
| // Spawn the process | ||
| int spawn_result = posix_spawn(&pid, "/bin/sh", &file_actions, nullptr, argv, environ); |
There was a problem hiding this comment.
Do we really need to run the shell here? It is seems like huge overhead.
There was a problem hiding this comment.
popen initiates a shell as well internally.
Without it, we need to complicate the code a bit, splitting the args in a vector, adding support for concatenating an env variable to the enviroment, and setting the execution dir explicitly.
But it it doable. Should I do it?
There was a problem hiding this comment.
As for "splitting the args in a vector" I don't think we really need to do it since each exec() client joins (through boost format) the arguments into a string. So, the clients can pass those arguments as an array.
Having said that I don't know whether it is worth doing.
src/exec.cc
Outdated
| if (stderr_open) FD_SET(stderr_pipe[0], &read_fds); | ||
|
|
||
| // Wait for data on either pipe (with timeout) | ||
| struct timeval timeout; |
There was a problem hiding this comment.
Why do we need the "timeout"? I think we should use either the system level timeout as we do now (i.e. timeout && ), or use the timeout in select instead of the system timeout. If so, then we should handle the timeout after select.
There was a problem hiding this comment.
I usually prefer to call select with timeout, but in that case it is not really required. I'm removing it.
detsch
force-pushed
the
detsch-popen-with-stderr
branch
2 times, most recently
from
765adfb to
52dfd34
Compare
|
Fixed |
detsch
force-pushed
the
detsch-popen-with-stderr
branch
from
52dfd34 to
c820d36
Compare
Current implementation of the exec function merges stdout and stderr, which may lead to problems when the output is meant to be parsed, but some error message is printed as well. Properly separate both outputs from the subprocess, allowing stderr to be ignored if the execution is successful, or logged in case of errors. Signed-off-by: Andre Detsch <andre.detsch@foundries.io>
Control execution timeout internally, leveraging select timeout. Signed-off-by: Andre Detsch <andre.detsch@foundries.io>
New exec implementation returns a different error when the command is not found. Signed-off-by: Andre Detsch <andre.detsch@foundries.io>
detsch
force-pushed
the
detsch-popen-with-stderr
branch
from
efa21c2 to
274a18e
Compare
|
@mike-sul I've added 2 more commits. Can you take a look again? |
3 participants
Current implementation of the exec function merges stdout and stderr, which may lead to problems when the output is meant to be parsed, but some error message is printed as well.
Properly separate both outputs from the subprocess, allowing stderr to be ignored if the execution is successful, or logged in case of errors.