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