You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/09/28 19:25:35 UTC

Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
-------

socket: refactor to use Option and fix file descriptor leaks.


Diffs
-----

  3rdparty/libprocess/include/process/socket.hpp a677e29b28711fc065134c11792b4f62fa3aa8b4 
  3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 

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


Testing
-------


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review101023
-----------------------------------------------------------

Ship it!


Looks great, thanks Chi!

- Ben Mahler


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Joris Van Remoortere <jo...@gmail.com>.

> On Sept. 29, 2015, 9:42 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 79-81
> > <https://reviews.apache.org/r/38809/diff/4/?file=1087354#file1087354line79>
> >
> >     We're using `s.get()` above here, so `owned` should never be `true`?
> 
> Chi Zhang wrote:
>     owned is determined upon entering the function? (we just reuse the variable name if s is none)

I see, it gets re-assigned.


- Joris


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.

> On Sept. 29, 2015, 9:42 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 79-81
> > <https://reviews.apache.org/r/38809/diff/4/?file=1087354#file1087354line79>
> >
> >     We're using `s.get()` above here, so `owned` should never be `true`?

owned is determined upon entering the function? (we just reuse the variable name if s is none)


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review101028
-----------------------------------------------------------



3rdparty/libprocess/src/socket.cpp (lines 79 - 81)
<https://reviews.apache.org/r/38809/#comment158331>

    We're using `s.get()` above here, so `owned` should never be `true`?



3rdparty/libprocess/src/socket.cpp (lines 91 - 93)
<https://reviews.apache.org/r/38809/#comment158332>

    We're using `s.get()` above here, so `owned` should never be `true`?


- Joris Van Remoortere


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/
-----------------------------------------------------------

(Updated Sept. 29, 2015, 8:30 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
-------

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/socket.hpp 817cb37 
  3rdparty/libprocess/src/socket.cpp 5879423 

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


Testing
-------

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100995
-----------------------------------------------------------


Patch looks great!

Reviews applied: [38809]

All tests passed.

- Mesos ReviewBot


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.

> On Sept. 29, 2015, 8:01 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, line 42
> > <https://reviews.apache.org/r/38809/diff/3/?file=1087196#file1087196line42>
> >
> >     newline :)

you meant adding one above it right?


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100999
-----------------------------------------------------------

Ship it!


Thanks Chi, just some minor cleanups to style. Once updated, I'll get this committed.


3rdparty/libprocess/src/socket.cpp (lines 39 - 40)
<https://reviews.apache.org/r/38809/#comment158301>

    In general we try to wrap comments so as to avoid jaggedness, e.g.
    
    ```
      // If the caller passed in a file descriptor, we do not own its life
      // cycle and must not close it.
    ```
    
    ```
      // If the caller passed in a file descriptor, we do
      // not own its life cycle and must not close it.
    ```
    
    The latter is easier on the eyes :)



3rdparty/libprocess/src/socket.cpp (line 42)
<https://reviews.apache.org/r/38809/#comment158302>

    newline :)


- Ben Mahler


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/
-----------------------------------------------------------

(Updated Sept. 29, 2015, 6:11 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
-------

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/socket.hpp 817cb37 
  3rdparty/libprocess/src/socket.cpp 5879423 

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


Testing
-------

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100915
-----------------------------------------------------------


Patch looks great!

Reviews applied: [38809]

All tests passed.

- Mesos ReviewBot


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.

> On Sept. 29, 2015, 12:47 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 42-44
> > <https://reviews.apache.org/r/38809/diff/2/?file=1086106#file1086106line42>
> >
> >     Just as an aside, it's unfortunate the caller has to do this, we should consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC:
> >     
> >     https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33

I actually think this is not too bad -- a very staightforward if-else. 

I can probably break it up and save one network::socket call -- not sure if that's worth the added complexity, but I can do it in another patch if you'd like to see that?


- Chi


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


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.

> On Sept. 29, 2015, 12:47 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 42-44
> > <https://reviews.apache.org/r/38809/diff/2/?file=1086106#file1086106line42>
> >
> >     Just as an aside, it's unfortunate the caller has to do this, we should consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC:
> >     
> >     https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33
> 
> Chi Zhang wrote:
>     I actually think this is not too bad -- a very staightforward if-else. 
>     
>     I can probably break it up and save one network::socket call -- not sure if that's worth the added complexity, but I can do it in another patch if you'd like to see that?
> 
> Ben Mahler wrote:
>     No don't do it right now, just an aside. While it's straightforward here, every user of network::socket is going to have this #ifdef which complicates the calling code, it got out of hand for os::open which is why we provided O_CLOEXEC for all platforms.

okay, you meant pushing all this into network::socket. Yeah that makes sense.


- Chi


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


On Sept. 29, 2015, 8:30 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 8:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Ben Mahler <be...@gmail.com>.

> On Sept. 29, 2015, 12:47 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/socket.cpp, lines 42-44
> > <https://reviews.apache.org/r/38809/diff/2/?file=1086106#file1086106line42>
> >
> >     Just as an aside, it's unfortunate the caller has to do this, we should consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC:
> >     
> >     https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33
> 
> Chi Zhang wrote:
>     I actually think this is not too bad -- a very staightforward if-else. 
>     
>     I can probably break it up and save one network::socket call -- not sure if that's worth the added complexity, but I can do it in another patch if you'd like to see that?

No don't do it right now, just an aside. While it's straightforward here, every user of network::socket is going to have this #ifdef which complicates the calling code, it got out of hand for os::open which is why we provided O_CLOEXEC for all platforms.


- Ben


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


On Sept. 29, 2015, 6:11 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 6:11 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb37 
>   3rdparty/libprocess/src/socket.cpp 5879423 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100909
-----------------------------------------------------------



3rdparty/libprocess/include/process/socket.hpp (lines 64 - 65)
<https://reviews.apache.org/r/38809/#comment158168>

    Can you fix the wrapping here? Also, I would suggest s/socketfd/s/ to keep the same as before, since `socketfd` is a bit unusual for our code base. Also the doxygen a few lines up refers to 's' still :)



3rdparty/libprocess/src/socket.cpp (lines 42 - 44)
<https://reviews.apache.org/r/38809/#comment158169>

    Just as an aside, it's unfortunate the caller has to do this, we should consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC:
    
    https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33



3rdparty/libprocess/src/socket.cpp (lines 77 - 79)
<https://reviews.apache.org/r/38809/#comment158171>

    How about we store a bool for this at the top instead of having multiple variables?
    
    ```
    // If the caller passed in a descriptor, we do
    // not own its lifecycle and must not close it!
    bool owned = s.isNone();
    
    ...
    ```


- Ben Mahler


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/
-----------------------------------------------------------

(Updated Sept. 28, 2015, 9:07 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
-------

socket: refactor to use Option and fix file descriptor leaks.


Diffs (updated)
-----

  3rdparty/libprocess/include/process/socket.hpp 817cb3711742871630a13b966563296644008f21 
  3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 

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


Testing (updated)
-------

make check


Thanks,

Chi Zhang

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Chi Zhang <ch...@gmail.com>.

> On Sept. 28, 2015, 6:03 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/socket.cpp, line 77
> > <https://reviews.apache.org/r/38809/diff/1/?file=1085970#file1085970line77>
> >
> >     Why is this conditional on socketFd?

The idea is I am only responsible for the resource I allocated. If a socketfd is passed in, I leave it to the owner to free it; if a socketfd is created by me, I will free it.


- Chi


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


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Neil Conway <ne...@gmail.com>.

> On Sept. 28, 2015, 6:03 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/socket.cpp, line 77
> > <https://reviews.apache.org/r/38809/diff/1/?file=1085970#file1085970line77>
> >
> >     Why is this conditional on socketFd?
> 
> Chi Zhang wrote:
>     The idea is I am only responsible for the resource I allocated. If a socketfd is passed in, I leave it to the owner to free it; if a socketfd is created by me, I will free it.

Duh, makes sense.


- Neil


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


On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 9:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp 817cb3711742871630a13b966563296644008f21 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100831
-----------------------------------------------------------



3rdparty/libprocess/src/socket.cpp (line 40)
<https://reviews.apache.org/r/38809/#comment158076>

    "socketfd.isNone()" would be more readable, IMHO.



3rdparty/libprocess/src/socket.cpp (line 77)
<https://reviews.apache.org/r/38809/#comment158079>

    Why is this conditional on socketFd?



3rdparty/libprocess/src/socket.cpp (line 89)
<https://reviews.apache.org/r/38809/#comment158080>

    Same as above.


- Neil Conway


On Sept. 28, 2015, 5:25 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp a677e29b28711fc065134c11792b4f62fa3aa8b4 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 38809: socket: refactor to use Option and fix file descriptor leaks.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38809/#review100854
-----------------------------------------------------------


Patch looks great!

Reviews applied: [38809]

All tests passed.

- Mesos ReviewBot


On Sept. 28, 2015, 5:25 p.m., Chi Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38809/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3519
>     https://issues.apache.org/jira/browse/MESOS-3519
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> socket: refactor to use Option and fix file descriptor leaks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/socket.hpp a677e29b28711fc065134c11792b4f62fa3aa8b4 
>   3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 
> 
> Diff: https://reviews.apache.org/r/38809/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>