Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Jörg Schad]

Only '../configure --with-network-isolator' which unfortunately wasn't the
case on neither of the system..
Sorry for the inconvenience, fix is just being tested.

On Mon, Mar 28, 2016 at 7:15 PM, Cong Wang  wrote:

> I am wondering how does it pass your test since you did `make check`
> on both Linux and OSX? It fails immediately for me on Linux...
>
>
> On Mon, Mar 28, 2016 at 10:09 AM, Joris Van Remoortere
>  wrote:
> > Joerg will fix these.
> > Thanks!
> >
> > On Mon, Mar 28, 2016 at 7:06 PM, Cong Wang 
> wrote:
> >
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/45230/
> >> 3rdparty/libprocess/include/process/subprocess.hpp
> >> <
> https://reviews.apache.org/r/45230/diff/2/?file=1316622#file1316622line311>
> (Diff
> >> revision 2)
> >>
> >> public:
> >>
> >> 301
> >>
> >> const Setsid setsid = NO_SETSID,
> >>
> >> This one breaks build even after all of your 7 patches are committed,
> because port_mapping.cpp passes flags but no setsid to subprocess(), this
> is not allowed by C++.
> >>
> >>
> >> 3rdparty/libprocess/include/process/subprocess.hpp
> >> <
> https://reviews.apache.org/r/45230/diff/2/?file=1316622#file1316622line356>
> (Diff
> >> revision 2)
> >>
> >> public:
> >>
> >> 342
> >>
> >> const Setsid setsid = NO_SETSID,
> >>
> >> Ditto
> >>
> >>
> >> - Cong Wang
> >>
> >> On March 28th, 2016, 4:51 p.m. UTC, Joerg Schad wrote:
> >> Review request for mesos and Joris Van Remoortere.
> >> By Joerg Schad.
> >>
> >> *Updated March 28, 2016, 4:51 p.m.*
> >> *Bugs: * MESOS-5049 
> >> *Repository: * mesos
> >> Description
> >>
> >> Executing arbitrary setup functions while creating new processes is
> >> dangerous as all functions called have to be async safe. As setup
> >> functions are used for only very few purposes (setsid, chdir, monitoring
> >> and killing a process (see upcoming review) it makes sense to support
> >> them safely via parameters to subprocess. Note this review by itself
> >> -without the following ones- removing the uses of the old interface will
> >> break the build.
> >>
> >> Testing
> >>
> >> tested entire chain (see https://reviews.apache.org/r/45236/).
> >>
> >> Diffs
> >>
> >>- 3rdparty/libprocess/include/process/ssl/gtest.hpp
> >>(2ca705524c8f9bba3c03eef296dc04a353dd236c)
> >>- 3rdparty/libprocess/include/process/subprocess.hpp
> >>(e0c306aa5cf5f393abb73768bbd287c45730f076)
> >>- 3rdparty/libprocess/src/subprocess.cpp
> >>(b99bad04f7251169df3bfcec5dee459977440997)
> >>
> >> View Diff 
> >>
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joerg Schad]

> On March 28, 2016, 5:06 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 311
> > 
> >
> > This one breaks build even after all of your 7 patches are committed, 
> > because port_mapping.cpp passes flags but no setsid to subprocess(), this 
> > is not allowed by C++.
> 
> Joerg Schad wrote:
> Thx for catching: Fixed with https://reviews.apache.org/r/45400/

Actually: https://reviews.apache.org/r/45399/


- Joerg


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


On March 28, 2016, 4:51 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45230/
> ---
> 
> (Updated March 28, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5049
> https://issues.apache.org/jira/browse/MESOS-5049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executing arbitrary setup functions while creating new processes is
> dangerous as all functions called have to be async safe. As setup
> functions are used for only very few purposes (setsid, chdir, monitoring
> and killing a process (see upcoming review) it makes sense to support
> them safely via parameters to subprocess. Note this review by itself
> -without the following ones- removing the uses of the old interface will
> break the build.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ca705524c8f9bba3c03eef296dc04a353dd236c 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> e0c306aa5cf5f393abb73768bbd287c45730f076 
>   3rdparty/libprocess/src/subprocess.cpp 
> b99bad04f7251169df3bfcec5dee459977440997 
> 
> Diff: https://reviews.apache.org/r/45230/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain (see https://reviews.apache.org/r/45236/).
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joerg Schad]

> On March 28, 2016, 5:06 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 311
> > 
> >
> > This one breaks build even after all of your 7 patches are committed, 
> > because port_mapping.cpp passes flags but no setsid to subprocess(), this 
> > is not allowed by C++.

Thx for catching: Fixed with https://reviews.apache.org/r/45400/


- Joerg


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


On March 28, 2016, 4:51 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45230/
> ---
> 
> (Updated March 28, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5049
> https://issues.apache.org/jira/browse/MESOS-5049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executing arbitrary setup functions while creating new processes is
> dangerous as all functions called have to be async safe. As setup
> functions are used for only very few purposes (setsid, chdir, monitoring
> and killing a process (see upcoming review) it makes sense to support
> them safely via parameters to subprocess. Note this review by itself
> -without the following ones- removing the uses of the old interface will
> break the build.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ca705524c8f9bba3c03eef296dc04a353dd236c 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> e0c306aa5cf5f393abb73768bbd287c45730f076 
>   3rdparty/libprocess/src/subprocess.cpp 
> b99bad04f7251169df3bfcec5dee459977440997 
> 
> Diff: https://reviews.apache.org/r/45230/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain (see https://reviews.apache.org/r/45236/).
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Cong Wang]

I am wondering how does it pass your test since you did `make check`
on both Linux and OSX? It fails immediately for me on Linux...


On Mon, Mar 28, 2016 at 10:09 AM, Joris Van Remoortere
 wrote:
> Joerg will fix these.
> Thanks!
>
> On Mon, Mar 28, 2016 at 7:06 PM, Cong Wang  wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/45230/
>> 3rdparty/libprocess/include/process/subprocess.hpp
>>  
>> (Diff
>> revision 2)
>>
>> public:
>>
>> 301
>>
>> const Setsid setsid = NO_SETSID,
>>
>> This one breaks build even after all of your 7 patches are committed, 
>> because port_mapping.cpp passes flags but no setsid to subprocess(), this is 
>> not allowed by C++.
>>
>>
>> 3rdparty/libprocess/include/process/subprocess.hpp
>>  
>> (Diff
>> revision 2)
>>
>> public:
>>
>> 342
>>
>> const Setsid setsid = NO_SETSID,
>>
>> Ditto
>>
>>
>> - Cong Wang
>>
>> On March 28th, 2016, 4:51 p.m. UTC, Joerg Schad wrote:
>> Review request for mesos and Joris Van Remoortere.
>> By Joerg Schad.
>>
>> *Updated March 28, 2016, 4:51 p.m.*
>> *Bugs: * MESOS-5049 
>> *Repository: * mesos
>> Description
>>
>> Executing arbitrary setup functions while creating new processes is
>> dangerous as all functions called have to be async safe. As setup
>> functions are used for only very few purposes (setsid, chdir, monitoring
>> and killing a process (see upcoming review) it makes sense to support
>> them safely via parameters to subprocess. Note this review by itself
>> -without the following ones- removing the uses of the old interface will
>> break the build.
>>
>> Testing
>>
>> tested entire chain (see https://reviews.apache.org/r/45236/).
>>
>> Diffs
>>
>>- 3rdparty/libprocess/include/process/ssl/gtest.hpp
>>(2ca705524c8f9bba3c03eef296dc04a353dd236c)
>>- 3rdparty/libprocess/include/process/subprocess.hpp
>>(e0c306aa5cf5f393abb73768bbd287c45730f076)
>>- 3rdparty/libprocess/src/subprocess.cpp
>>(b99bad04f7251169df3bfcec5dee459977440997)
>>
>> View Diff 
>>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joris Van Remoortere]

Joerg will fix these.
Thanks!

On Mon, Mar 28, 2016 at 7:06 PM, Cong Wang  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45230/
> 3rdparty/libprocess/include/process/subprocess.hpp
>  
> (Diff
> revision 2)
>
> public:
>
> 301
>
> const Setsid setsid = NO_SETSID,
>
> This one breaks build even after all of your 7 patches are committed, because 
> port_mapping.cpp passes flags but no setsid to subprocess(), this is not 
> allowed by C++.
>
>
> 3rdparty/libprocess/include/process/subprocess.hpp
>  
> (Diff
> revision 2)
>
> public:
>
> 342
>
> const Setsid setsid = NO_SETSID,
>
> Ditto
>
>
> - Cong Wang
>
> On March 28th, 2016, 4:51 p.m. UTC, Joerg Schad wrote:
> Review request for mesos and Joris Van Remoortere.
> By Joerg Schad.
>
> *Updated March 28, 2016, 4:51 p.m.*
> *Bugs: * MESOS-5049 
> *Repository: * mesos
> Description
>
> Executing arbitrary setup functions while creating new processes is
> dangerous as all functions called have to be async safe. As setup
> functions are used for only very few purposes (setsid, chdir, monitoring
> and killing a process (see upcoming review) it makes sense to support
> them safely via parameters to subprocess. Note this review by itself
> -without the following ones- removing the uses of the old interface will
> break the build.
>
> Testing
>
> tested entire chain (see https://reviews.apache.org/r/45236/).
>
> Diffs
>
>- 3rdparty/libprocess/include/process/ssl/gtest.hpp
>(2ca705524c8f9bba3c03eef296dc04a353dd236c)
>- 3rdparty/libprocess/include/process/subprocess.hpp
>(e0c306aa5cf5f393abb73768bbd287c45730f076)
>- 3rdparty/libprocess/src/subprocess.cpp
>(b99bad04f7251169df3bfcec5dee459977440997)
>
> View Diff 
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Cong Wang]

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




3rdparty/libprocess/include/process/subprocess.hpp (line 301)


This one breaks build even after all of your 7 patches are committed, 
because port_mapping.cpp passes flags but no setsid to subprocess(), this is 
not allowed by C++.



3rdparty/libprocess/include/process/subprocess.hpp (line 342)


Ditto


- Cong Wang


On March 28, 2016, 4:51 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45230/
> ---
> 
> (Updated March 28, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5049
> https://issues.apache.org/jira/browse/MESOS-5049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executing arbitrary setup functions while creating new processes is
> dangerous as all functions called have to be async safe. As setup
> functions are used for only very few purposes (setsid, chdir, monitoring
> and killing a process (see upcoming review) it makes sense to support
> them safely via parameters to subprocess. Note this review by itself
> -without the following ones- removing the uses of the old interface will
> break the build.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ca705524c8f9bba3c03eef296dc04a353dd236c 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> e0c306aa5cf5f393abb73768bbd287c45730f076 
>   3rdparty/libprocess/src/subprocess.cpp 
> b99bad04f7251169df3bfcec5dee459977440997 
> 
> Diff: https://reviews.apache.org/r/45230/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain (see https://reviews.apache.org/r/45236/).
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joerg Schad]

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

(Updated March 28, 2016, 4:51 p.m.)


Review request for mesos and Joris Van Remoortere.


Bugs: MESOS-5049
https://issues.apache.org/jira/browse/MESOS-5049


Repository: mesos


Description
---

Executing arbitrary setup functions while creating new processes is
dangerous as all functions called have to be async safe. As setup
functions are used for only very few purposes (setsid, chdir, monitoring
and killing a process (see upcoming review) it makes sense to support
them safely via parameters to subprocess. Note this review by itself
-without the following ones- removing the uses of the old interface will
break the build.


Diffs
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
2ca705524c8f9bba3c03eef296dc04a353dd236c 
  3rdparty/libprocess/include/process/subprocess.hpp 
e0c306aa5cf5f393abb73768bbd287c45730f076 
  3rdparty/libprocess/src/subprocess.cpp 
b99bad04f7251169df3bfcec5dee459977440997 

Diff: https://reviews.apache.org/r/45230/diff/


Testing
---

tested entire chain (see https://reviews.apache.org/r/45236/).


Thanks,

Joerg Schad

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joris Van Remoortere]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/subprocess.hpp (line 39)


brace on new line.
```
enum Setsid
{
  ...
};
```



3rdparty/libprocess/include/process/subprocess.hpp (line 131)


`set_sid`
and elsehwere.



3rdparty/libprocess/include/process/subprocess.hpp (line 280)


ticks around `'parent_hooks'`
and elsewhere.



3rdparty/libprocess/src/subprocess.cpp (line 315)


s/slave's/caller's



3rdparty/libprocess/src/subprocess.cpp (line 350)


no need for `std::`


- Joris Van Remoortere


On March 28, 2016, 11:42 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45230/
> ---
> 
> (Updated March 28, 2016, 11:42 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executing arbitrary setup functions while creating new processes is
> dangerous as all functions called have to be async safe. As setup
> functions are used for only very few purposes (setsid, chdir, monitoring
> and killing a process (see upcoming review) it makes sense to support
> them safely via parameters to subprocess. Note this review by itself
> -without the following ones- removing the uses of the old interface will
> break the build.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ca705524c8f9bba3c03eef296dc04a353dd236c 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> e0c306aa5cf5f393abb73768bbd287c45730f076 
>   3rdparty/libprocess/src/subprocess.cpp 
> b99bad04f7251169df3bfcec5dee459977440997 
> 
> Diff: https://reviews.apache.org/r/45230/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain (see https://reviews.apache.org/r/45236/).
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45230: Refactored subproces setup functions [1/7].

[Joerg Schad]

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

(Updated March 28, 2016, 11:42 a.m.)


Review request for mesos and Joris Van Remoortere.


Summary (updated)
-

Refactored subproces setup functions [1/7].


Repository: mesos


Description
---

Executing arbitrary setup functions while creating new processes is
dangerous as all functions called have to be async safe. As setup
functions are used for only very few purposes (setsid, chdir, monitoring
and killing a process (see upcoming review) it makes sense to support
them safely via parameters to subprocess. Note this review by itself
-without the following ones- removing the uses of the old interface will
break the build.


Diffs
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
2ca705524c8f9bba3c03eef296dc04a353dd236c 
  3rdparty/libprocess/include/process/subprocess.hpp 
e0c306aa5cf5f393abb73768bbd287c45730f076 
  3rdparty/libprocess/src/subprocess.cpp 
b99bad04f7251169df3bfcec5dee459977440997 

Diff: https://reviews.apache.org/r/45230/diff/


Testing
---

tested entire chain (see https://reviews.apache.org/r/45236/).


Thanks,

Joerg Schad