> On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 56 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line56> > > > > Could you explain in what situations and why the list of arguments > > could/would be modified...?
I think I should remove this comment (in fact, I just did): originally, I was hoping to be able to somehow fix the awkward situation in which `arg[0]` is just the `progname` and not actually used as a command argument: while this is "obvious" for any C/C++ developer, it is also undocumented in `os::shell` and leads to unexpected behavior for unaware users of `CommandInfo` (for example). It turns out that that's not possible without breaking a lot of existing code/frameworks, so I guess it's here to stay. > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 120 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line120> > > > > 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? Done. Renamed `Command` -> `Invocation` Renamed `CommandResult` -> `Subprocess::Result` Thanks for the suggestion! (I was unhappy about naming too) > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 135-152 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line135> > > > > 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`? It is safe to assume that every shell command will *always* emit to `stdout` (possibly an empty string) - while, not necessarily so to `stderr`. It is probably a "distinction without a difference" but I believe it makes the caller's code easier to write and removes unnecessary (and tedious) `.isSome()` and `.get()` for stdout; while allowing for the possibility that there is no `stderr` available. Obviously, we could make `stderr_` a `string` too - but making it an `Option` seemed to me to be more in line with Mesos' practices. > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 400 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113944#file1113944line400> > > > > Is there a reason why this is wrapped in a `std::shared_ptr`? yes, because I need to pass it to the lambda and not doing so seemed to cause issues (if memory serves). Wrapping it in a `shared_ptr` (instead of using a `raw` pointer) is also, in my understanding, "best practice" now in C++11? > On Nov. 3, 2015, 12:55 a.m., Michael Park wrote: > > 3rdparty/libprocess/src/subprocess.cpp, lines 499-503 > > <https://reviews.apache.org/r/37336/diff/9/?file=1113945#file1113945line499> > > > > 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));` well, `invokedWith_` is already "exposed" to `subprocess` - as it's a 'friend' anyway. But I have simplified the invocation, so that it reflects closely what you suggested. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review104816 ----------------------------------------------------------- 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 > >
