> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, line > > 73 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175214#file1175214line73> > > > > How semantically similar is this to the POSIX code? If it's very > > similar, does it make sense to transition the POSIX code to use the POSIX > > equivalent too?
Posix equivalne uses ::fork(). We cannot use that call. In theroy should have the same bahaviour. > On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, line > > 68 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175214#file1175214line68> > > > > The comments on the POSIX version of this function claims that it's > > async signal safe. Is that true of this version as well? Does that even > > make sense in Windows? We don't support POSIX signals in Windows. The comment is not need it. > On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, > > lines 25-28 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175214#file1175214line25> > > > > Historically this stuff has gone in `stout/windows.hpp`, so that if > > another function needs them, because (1) they don't all have to redefine > > the symbols, and (2) we wanted to keep all the Windows-compat stuff in one > > place until we had a clearer picture of what we wanted to do with this > > stuff in the long term. > > > > In the long term, I think the answer is that we've already started > > moving toward a model where each compat header gets its own file (_e.g._, > > `dirent.h` gets `stout/internal/windows/dirent.hpp`), so I think we'll > > eventually come back and refactor all of `windows.hpp` to fit this model. > > > > For the time being, though, I think it makes sense to just move this > > there. I see similar file with platform defines in libprocess/src/config.hpp. Maybe we should use a single file with all the types/define missing in windows. I will keep this in mind for further refactoring. > On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line > > 120 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175213#file1175213line120> > > > > Sorry, template noob here. Why the variadic template here? Why not: > > > > ``` > > int execlp(const char* path, const char* arg, ...) > > { > > return ::execlp(path, arg); > > } > > ``` > > > > My understanding here is that `T` could be any number of type > > arguments, and the result of `const T*... t` is that you could pass in a > > bunch of different types into the `os::execlp` function. > > > > Is that all correct? > > > > If so, then I don't really understand what this freedom buys us, since > > it seems like in practice we'll only call it with `const char*`. Removed the code. This is not requered > On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line > > 110 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175213#file1175213line110> > > > > `arg0` and `arg1` seem like they might not be the best names for these. > > Perhaps something more descriptive? The code has been removed. arg0 and arg1 follows the convention on the API param naming. > On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, lines > > 104-118 > > <https://reviews.apache.org/r/40936/diff/3/?file=1175213#file1175213line104> > > > > Looks like `shell_const` is used later, for commit of launch.cpp, right? > > > > I'm fine with that, but I have a couple suggestions: > > > > * Can we please indent the static members according to the style we use > > for structs? > > * Brief search of the codebase reveals that calls to `exec*("sh", "sh", > > ...)` are not that numerous. By my count, there are a few in fork.hpp, > > launch.cpp, executor.cpp, and here in shell.hpp, on line 144. It seems like > > it's worth just transitioning them all right now, since they're not that > > numerous, and since it seems bad to have the code in an inconsistent state. > > What do you think? We should mmake the transition when we add the files. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40936/#review111814 ----------------------------------------------------------- On Jan. 13, 2016, 7:14 p.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40936/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2016, 7:14 p.m.) > > > Review request for mesos, Alex Naparu, Alex Clemmer, and M Lawindi. > > > Repository: mesos > > > Description > ------- > > Windows: Unified POSIX and Windows implementation of shell.hpp > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp > b18cae1118302e18d2cfb7ce4089ab5079a01d1a > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp > bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 > 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp > 4cf693fb7e8c6bb3ad1920ebe90d61f0adb5dc99 > > Diff: https://reviews.apache.org/r/40936/diff/ > > > Testing > ------- > > > Thanks, > > Daniel Pravat > >
