mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 54351: Allowed subprocess to take duplicated FDs.
Date Mon, 05 Dec 2016 01:36:00 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54351/#comment228601>

    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)
<https://reviews.apache.org/r/54351/#comment228603>

    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)
<https://reviews.apache.org/r/54351/#comment228604>

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


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