Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:36 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Addressed comments. Rebased.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Benjamin Hindman]

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


Ship it!




The changes all make sense, but I agree with Kevin that moving the "ownership" 
for Windows into `internal::createChildProcess` would be more symetrical. 
Alternatively, could you move the close out from `cloneChild` on Linux and do 
the closes immediately after you return from both the Linux and Windows 
functions before you even check if the child creation was an error?

- Benjamin Hindman


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

> On Dec. 5, 2016, 1:36 a.m., Kevin Klues wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 330-344
> > 
> >
> > I feel like moving this into the windows `createChildProcess()` 
> > function would make it more semetrical with the change above. It seems odd 
> > that we would ahve to close out here on windows, but not on posix.
> > 
> > Also, lines 318 and 326 above should either be removed (if you follow 
> > my suggestion) or changed to be the same as this line (if you don't).

This part the code needs to be refactored. The logic is really hard to follow 
when I did the change. I'll add a TODO for now because I don't have a good way 
to test windows code.


- Jie


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


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

> On Dec. 5, 2016, 1:36 a.m., Kevin Klues wrote:
> > 3rdparty/libprocess/include/process/posix/subprocess.hpp, lines 332-384
> > 
> >
> > Is this move actually related to this commit, or is it just an existing 
> > bug fix / simplification?

I need to change those anyway because those FDs might be equal. Therefore, I 
did this simplication.


- Jie


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


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Kevin Klues]

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




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


What if `stdoutfds.write == stderrfds.write`? I guess you will close it 
here, and then *not* close it in the next if statement?

Looking more closely at it, it seems like you just extend the (implicit) 
logic described in the comment for *not* closing stdin/stdout/stderr because 
they have already been closed by the dup call.

I'd extend the comment to make it clear what's going on.

Simething like:
```
...
We also need to ensure that we don't "double close" any file descriptors in 
the case where one of stdinfds.read, stdoutfds.write, or stdoutfds.write are 
equal.
```
```



3rdparty/libprocess/include/process/posix/subprocess.hpp (lines 329 - 370)


Is this move actually related to this commit, or is it just an existing bug 
fix / simplification?



3rdparty/libprocess/src/subprocess.cpp (lines 330 - 338)


I feel like moving this into the windows `createChildProcess()` function 
would make it more semetrical with the change above. It seems odd that we would 
ahve to close out here on windows, but not on posix.

Also, lines 318 and 326 above should either be removed (if you follow my 
suggestion) or changed to be the same as this line (if you don't).


- Kevin Klues


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

(Updated Dec. 5, 2016, 12:16 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 

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


Testing
---

make check


Thanks,

Jie Yu