----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review104816 -----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (line 56) <https://reviews.apache.org/r/37336/#comment163060> Could you explain in what situations and why the list of arguments could/would be modified...? 3rdparty/libprocess/include/process/subprocess.hpp (lines 62 - 67) <https://reviews.apache.org/r/37336/#comment163061> `= default;` works fine here. Just a future note: `for(std::string arg : other.args)` creates a copy for every string, which is then copied again on `args.push_back(arg)`. Using `std::vector`'s copy constructor would've been better, `= default;` would be better. You could even just omit it entirely if you wish, in which case it's equivalent to `= default;` 3rdparty/libprocess/include/process/subprocess.hpp (line 70) <https://reviews.apache.org/r/37336/#comment163063> `= {}` works for default construction. `:` should be on the next line. 3rdparty/libprocess/include/process/subprocess.hpp (lines 73 - 75) <https://reviews.apache.org/r/37336/#comment163065> Please differentiate the name between `this->args` and `args`, and simply use `std::vector`'s copy constructor rather than manually looping. 3rdparty/libprocess/include/process/subprocess.hpp (line 100) <https://reviews.apache.org/r/37336/#comment163066> This should be just `f.get();` right? 3rdparty/libprocess/include/process/subprocess.hpp (line 113) <https://reviews.apache.org/r/37336/#comment163082> `:` on the next line. 3rdparty/libprocess/include/process/subprocess.hpp (line 120) <https://reviews.apache.org/r/37336/#comment163069> This suggests to me either that `Command` should be named `Invocation`, or that this variable should be named `command`. Given that `Command` is storing both the command and the arguments, it seems more fitting to name it `Invocation`. Additionally, I think it would make a lot of sense to simply name it `Result` and move it into the `Subprocess` class. Similar to `Subprocess::IO`, we would have `Subprocess::Result` which captures the result of a subprocess call. What do you think? 3rdparty/libprocess/include/process/subprocess.hpp (lines 135 - 152) <https://reviews.apache.org/r/37336/#comment163095> Could you explain why these aren't symmetric? That is, why is `stdout_` not a `Option<std::string>` or why is `stderr_` not a `std::string`? 3rdparty/libprocess/include/process/subprocess.hpp (line 293) <https://reviews.apache.org/r/37336/#comment163079> `s/writing to the stream/writing to stderr/` To keep consistency with the `stdout` version above. 3rdparty/libprocess/include/process/subprocess.hpp (line 400) <https://reviews.apache.org/r/37336/#comment163089> Is there a reason why this is wrapped in a `std::shared_ptr`? 3rdparty/libprocess/src/subprocess.cpp (line 219) <https://reviews.apache.org/r/37336/#comment163092> Can we not just call `cleanup()` here? 3rdparty/libprocess/src/subprocess.cpp (line 225) <https://reviews.apache.org/r/37336/#comment163093> Remove newline. 3rdparty/libprocess/src/subprocess.cpp (line 254) <https://reviews.apache.org/r/37336/#comment163091> Indented 1 level too deep. 3rdparty/libprocess/src/subprocess.cpp (lines 499 - 503) <https://reviews.apache.org/r/37336/#comment163090> Rather than exposing the `invokedWith_` member variable like this, can we introduce a `Subprocess` constructor that takes a `Command` instead? At which point we can do something like: `Subprocess process(path == "sh" ? Command(argv[2]) : Command(path, argv));` - Michael Park On Nov. 2, 2015, 7:22 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2015, 7:22 a.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-3035 > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added an `execute()` method > that returns a `Future<CommandResult>` (also introduced with this patch) > which > contains useful information about the command invocation; the exit code; > stdout and, optionally, stderr too. > > Once the Future completes, if successful, the caller will be able to > retrieve > stdout/stderr; whether the command was successful; and whether it > received a signal > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > f17816e813d5efce1d3bb1ff1e1111850eeda3ba > 3rdparty/libprocess/src/subprocess.cpp > 459825c188d56d25f6e2832e5a170d806e257d6b > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ac600a551fb1a7782ff33cce204b7819497ef54a > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
