You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2014/11/17 19:47:51 UTC
Review Request 28125: Fix compilation error for Socket on older linux
+ OSX.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-2116
https://issues.apache.org/jira/browse/MESOS-2116
Repository: mesos-git
Description
-------
Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
Otherwise use os::nonblock and os::cloexec after creating the socket.
Diffs
-----
3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
Diff: https://reviews.apache.org/r/28125/diff/
Testing
-------
make check on linux
make check on macos
Thanks,
Joris Van Remoortere
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61773
-----------------------------------------------------------
Ship it!
LGTM modulo Ben's comment
We could think about making process::socket more robust and introduce our own SocketOption enum - that would make it easier to read and harder to use wrong :-)
- Niklas Nielsen
On Nov. 17, 2014, 10:47 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 10:47 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Nov. 17, 2014, 6:49 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 95
> > <https://reviews.apache.org/r/28125/diff/1/?file=765990#file765990line95>
> >
> > why not just do the second version all the time?
Because making 1 system call instead of 3 is preferred. That is why this option was added to the system call itself.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61762
-----------------------------------------------------------
On Nov. 17, 2014, 6:47 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 6:47 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Nov. 17, 2014, 10:49 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 95
> > <https://reviews.apache.org/r/28125/diff/1/?file=765990#file765990line95>
> >
> > why not just do the second version all the time?
>
> Joris Van Remoortere wrote:
> Because making 1 system call instead of 3 is preferred. That is why this option was added to the system call itself.
I understand why the single case is preferred, but given we can't support that in all cases, this just adds extra noise and reduces readability. Just use the long form please to cover every case.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61762
-----------------------------------------------------------
On Nov. 17, 2014, 10:47 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 10:47 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61762
-----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103642>
why not just do the second version all the time?
- Dominic Hamon
On Nov. 17, 2014, 10:47 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 10:47 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61775
-----------------------------------------------------------
Ship it!
Looks great!
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103661>
Please try to use newlines between blocks like this. Also, don't think this line of the comment adds much value?
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103664>
A newline here would reduce "jagedness" IMO, but up to you.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103663>
Fits one one line?
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103662>
How about a newline here?
- Ben Mahler
On Nov. 17, 2014, 7:06 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 7:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61785
-----------------------------------------------------------
Patch looks great!
Reviews applied: [28125]
All tests passed.
- Mesos ReviewBot
On Nov. 17, 2014, 7:26 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 7:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/
-----------------------------------------------------------
(Updated Nov. 17, 2014, 7:26 p.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
Changes
-------
Adjust whitespace.
Bugs: MESOS-2116
https://issues.apache.org/jira/browse/MESOS-2116
Repository: mesos-git
Description
-------
Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
Otherwise use os::nonblock and os::cloexec after creating the socket.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
Diff: https://reviews.apache.org/r/28125/diff/
Testing
-------
make check on linux
make check on macos
Thanks,
Joris Van Remoortere
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/
-----------------------------------------------------------
(Updated Nov. 17, 2014, 7:06 p.m.)
Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
Changes
-------
Update comments.
Bugs: MESOS-2116
https://issues.apache.org/jira/browse/MESOS-2116
Repository: mesos-git
Description
-------
Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
Otherwise use os::nonblock and os::cloexec after creating the socket.
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
Diff: https://reviews.apache.org/r/28125/diff/
Testing
-------
make check on linux
make check on macos
Thanks,
Joris Van Remoortere
Re: Review Request 28125: Fix compilation error for Socket on older
linux + OSX.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28125/#review61768
-----------------------------------------------------------
Thanks Joris! Feel free to have someone just get this committed with the minor adjustments.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103649>
IMO the added comments are unnecessary, not provided more insight over just reaing the code.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28125/#comment103647>
What does "link" mean in this context?
How about:
"Failed to create socket, os::nonblock: " + ...
Ditto below.
- Ben Mahler
On Nov. 17, 2014, 6:47 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28125/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2014, 6:47 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Niklas Nielsen, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-2116
> https://issues.apache.org/jira/browse/MESOS-2116
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Test whether SOCK_NONBLOCK and SOCK_CLOEXEC are defined to use single system call to create socket.
> Otherwise use os::nonblock and os::cloexec after creating the socket.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp 20136ba4c65883b8bbcc8498e09f7ff7b50b68a4
>
> Diff: https://reviews.apache.org/r/28125/diff/
>
>
> Testing
> -------
>
> make check on linux
> make check on macos
>
>
> Thanks,
>
> Joris Van Remoortere
>
>