> 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
> 
>

Reply via email to