-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94162
-----------------------------------------------------------

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 40)
<https://reviews.apache.org/r/36978/#comment148715>

    Was not it discussed at some point that it would be good to have a funtion 
that returns a {status, stderr, stdout} tuple?



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 50)
<https://reviews.apache.org/r/36978/#comment148697>

    should the variable be called `_cmd`?



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 57)
<https://reviews.apache.org/r/36978/#comment148703>

    Why not do `std::string cmd = _cmd + " " + strings::join(" ", args)?` and 
get rid of cmdLine?



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (lines 86 - 
88)
<https://reviews.apache.org/r/36978/#comment148705>

    alignment seems to be off here.



3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 87)
<https://reviews.apache.org/r/36978/#comment148708>

    Is there are reason for specifically calling out error 127? I would suggest 
a generic error message + stringify(WEXITSTATUS(status)).



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 881)
<https://reviews.apache.org/r/36978/#comment148714>

    Maybe it makes sense to factor out os::shell tests into a separate function.



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 882)
<https://reviews.apache.org/r/36978/#comment148713>

    Could you please add a test that makes sure that redirecting stderr to 
stdout with 2>&1 works?



3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 885)
<https://reviews.apache.org/r/36978/#comment148711>

    From what I've seen the variables with _ are used when some transformation 
needs to be applied on the original value:
    
    ```
    _var = transform1(__var);
    var = transform2(_var);
    ```


- Artem Harutyunyan


On Aug. 4, 2015, 5:54 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3142
>     https://issues.apache.org/jira/browse/MESOS-3142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactoring os::shell.
> See MESOS-3142 for more details.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
> 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 2556bd428cc8990659e30e804b9c96c1659ef4a1 
> 
> Diff: https://reviews.apache.org/r/36978/diff/
> 
> 
> Testing
> -------
> 
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: 
> see also https://reviews.apache.org/r/36979/
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Reply via email to