Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is the code I'm currently using to run a shell command on the current machine, intended for use only with GNU/Linux at the moment:

std::string getCmdOutput(const std::string& mStr)
{
    std::string result, file;
    FILE* pipe{popen(mStr.c_str(), "r")};
    char buffer[256];

    while(fgets(buffer, sizeof(buffer), pipe) != NULL)
    {
        file = buffer;
        result += file.substr(0, file.size() - 1);
    }

    pclose(pipe);
    return result;
}

Example use:

getCmdOutput(R"( sudo pacman -Syyuu )");
auto output(getCmdOutput(R"( echo /etc/pacman.conf )"));

I use this when making simple script-like C++ programs (mostly for personal use).

Is this the "correct" way of doing it? What can be improved?

share|improve this question
1  
In C++ I believe it is a bit more idiomatic to use fstream to open files instead of using FILE* which is a C API, but there doesn't appear to be anything wrong with what you're doing in that regard. –  YoungJohn Feb 19 at 15:11
    
The pipe opens up a two way pipe so you can write to standard input of the application you started. I would rather wrap it up inside an stream so you can read/write from/to the stream and it ends up going to the application. –  Loki Astari Feb 19 at 22:43
    
@YoungJohn: Could you please show me an example of using std::ifstream to get the output of a shell command? I tried using std::getline but I do not receive any output. –  Vittorio Romeo Feb 20 at 0:02
    
I would also like to know how to get the bash output into an ifstream. –  David Doria Mar 7 at 18:06
add comment

2 Answers

Prefer comparing pointers to nullptr instead of to NULL in C++11. nullptr exists for exactly that reason. The NULL macro is not portable. While it exists in many different environments there is no guarantee how it will be implemented or what it is intended to be used for. It may be implemented as 0, '\0', or as some pointer type, or anything else, and every different environment may define it differently or not at all. It is non-standard. nullptr is defined by the standard and all C++11 standard compliant compilers must support it.

share|improve this answer
add comment

Skipping the C++ specific parts, I would encourage you to do more for the error handling...

  • check the return value of popen!!! (nullptr for the FILE*)
  • check the return-value of pclose() If the pipe failed for any reason (including if there was an exit-code from the called script), the pclose() will be set to an error.
  • you are not trapping the STDERR of the process you call, which means you are not capturing/buffering the output, and you may end up with crap on your console, or missing data you expect, or both.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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