mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 46608: Libprocess: Implemented `subprocess_windows.cpp`.
Date Thu, 05 May 2016 01:05:15 GMT


> On April 28, 2016, 11:52 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/subprocess_windows.cpp, lines 534-539
> > <https://reviews.apache.org/r/46608/diff/1/?file=1358649#file1358649line534>
> >
> >     A few comments here.
> >     
> >     (1) I think we can simply this to:
> >     
> >     ```
> >     string env;
> >     foreach (const string& key, const string& value, environment.get())
{
> >       env += key + '=' + value + '\0';
> >     }
> >     ```
> >     
> >     then pass `env.data()` to `createChildProcess`.
> >     
> >     (2) Currently, if the string is larger than 32767 bytes, `createProcessEnvironment`
returns `NULL`. We subsequently pass that result to `createChildProcess`, which result in
the following behavior:
> >     
> >     > lpEnvironment [in, optional]
> >     A pointer to the environment block for the new process. If this parameter is
NULL, the new process uses the environment of the calling process.
> >     
> >     Is this what we want? I think we can definitely incorporate whatever behavior
we need if we go beyond the 32767 bytes, but I'm not sure exactly what behavior is expected
here.

We can do that. It was a bad failure mode to silently map environments that were too big to
`NULL`. We should let `::CreateProcess` error out in that case which will cause us to return
an `Error`.


- Alex


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


On May 3, 2016, 5:45 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46608/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 5:45 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere,
Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
>     https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Libprocess: Implemented `subprocess_windows.cpp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 
>   3rdparty/libprocess/src/subprocess.cpp bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46608/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


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