Skip to main content
added 33 characters in body
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

This is a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves whenever the function throws or returns.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

This is a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

This is a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves whenever the function throws or returns.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

Don't ignore WIFSIGNALED
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

This looksis a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

This looks reasonably good.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from).

This is a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

This looks reasonably good.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from).