mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Pravat" <dpra...@outlook.com>
Subject Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp
Date Wed, 13 Jan 2016 19:17:24 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message