You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Evelina Dumitrescu <ev...@gmail.com> on 2014/12/04 21:14:18 UTC
Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 5, 2014, 5:34 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1394
> > <https://reviews.apache.org/r/28716/diff/5/?file=783795#file783795line1394>
> >
> > return tryBind.error();
tryBind.error() is a string. Shouldn't it be the return value of Error type?
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64017
-----------------------------------------------------------
On Dec. 5, 2014, 5:11 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 5:11 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
> 3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 5, 2014, 5:34 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/net.hpp, line 49
> > <https://reviews.apache.org/r/28716/diff/5/?file=783794#file783794line49>
> >
> > that += seems weird.
> >
> > this should be:
> >
> > throw runtime_error("socket: " + trySocket.error());
> >
> > i think!
asked benh for more insight about the problem with the dead code.
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64017
-----------------------------------------------------------
On Dec. 5, 2014, 8:31 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 8:31 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64017
-----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106400>
can you try removing the explicit std::string constructor? it shouldn't be necessary as the rhs of the + is a std::string.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106402>
include the node in the error message please
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106401>
include the node in the error message please
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106403>
that += seems weird.
this should be:
throw runtime_error("socket: " + trySocket.error());
i think!
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106404>
throw runtime_error(tryAccept.error());
right?
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/28716/#comment106405>
maybe this would read easier if the condition was reversed:
if (errno == EINPROGRESS) {
return io::poll(get(), io::WRITE)
.then(lambda::bind(&internal::connect, Socket(shared_from_this())));
}
return Failure(tryConnect.error());
so it's clear that we're trying again if it's 'in progress' but failing otherwise.
what do you think?
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/28716/#comment106406>
return tryBind.error();
- Dominic Hamon
On Dec. 5, 2014, 9:11 a.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:11 a.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
> 3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 5, 2014, 5:55 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 85
> > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85>
> >
> > The pattern we use for succinctness and clarity in naming here is:
> >
> > ```
> > Try<int> connect = process::connect(...);
> > ```
> >
> > "try" is redundant here since it's encoded in the type.
> >
> > Ditto everywhere else. :)
>
> Evelina Dumitrescu wrote:
> See dhamon's last commit.
>
> Ben Mahler wrote:
> Ah thanks for pointing me to that! That looks better for sure, although I'm hoping to keep it even more consistent with how we've been writing things:
>
> ```
> Try<int> sock = process::socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
> ```
>
> I didn't expect to see any "socks" in our code. ;)
>
> Interestingly, looks like we leaked this pattern into the linux routing library:
>
> ```
> ? mesos git:(95752f8) ? grep -R 'sock ' src
> src/linux/routing/diagnosis/diagnosis.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(NETLINK_INET_DIAG);
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/route.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> ? mesos git:(95752f8) ? grep -R 'sock ' 3rdparty
> ```
>
> So ideally one of these:
>
> ```
> Try<int> socket = proces::socket(...);
> Try<int> s = proces::socket(...);
> ```
>
> and more generally on the other callsites, in these cases "rc" should have been "error":
>
> ```
> // Went from 'rc' -> 'accepted', but really we're storing the 'error':
> int accepted = ::accept(s, (sockaddr*) &addr, &addrlen);
> // Change to:
> int error = ::accept(s, (sockaddr*) &addr, &addrlen);
> ```
>
> ```
> // Went from 'rc' -> 'bound', but really we're storing the 'error':
> int bound = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> ```
> // Went from 'rc' -> 'connected', but really we're storing the 'error':
> int connected = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> Could you follow up on each of these Evelina? Thank you!!
>
> I'll comment on the other Try cases in https://reviews.apache.org/r/28789/ :)
>
> Ben Mahler wrote:
> Disregard the "accept" case, I missed that we're actually storing the "accepted" socket so the existing name makes sense. :)
>
> Evelina Dumitrescu wrote:
> Ok, thanks for explaining me all this :D! I will create a patch for fixing all the coding style issues.
> Should I make the following substitutions in the linux routing library?
>
> sock -> s/socket
> err -> error
if you do, please do it in another patch. it would be most welcome though :)
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 1:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 1:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 85
> > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85>
> >
> > The pattern we use for succinctness and clarity in naming here is:
> >
> > ```
> > Try<int> connect = process::connect(...);
> > ```
> >
> > "try" is redundant here since it's encoded in the type.
> >
> > Ditto everywhere else. :)
>
> Evelina Dumitrescu wrote:
> See dhamon's last commit.
>
> Ben Mahler wrote:
> Ah thanks for pointing me to that! That looks better for sure, although I'm hoping to keep it even more consistent with how we've been writing things:
>
> ```
> Try<int> sock = process::socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
> ```
>
> I didn't expect to see any "socks" in our code. ;)
>
> Interestingly, looks like we leaked this pattern into the linux routing library:
>
> ```
> ? mesos git:(95752f8) ? grep -R 'sock ' src
> src/linux/routing/diagnosis/diagnosis.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(NETLINK_INET_DIAG);
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/route.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> ? mesos git:(95752f8) ? grep -R 'sock ' 3rdparty
> ```
>
> So ideally one of these:
>
> ```
> Try<int> socket = proces::socket(...);
> Try<int> s = proces::socket(...);
> ```
>
> and more generally on the other callsites, in these cases "rc" should have been "error":
>
> ```
> // Went from 'rc' -> 'accepted', but really we're storing the 'error':
> int accepted = ::accept(s, (sockaddr*) &addr, &addrlen);
> // Change to:
> int error = ::accept(s, (sockaddr*) &addr, &addrlen);
> ```
>
> ```
> // Went from 'rc' -> 'bound', but really we're storing the 'error':
> int bound = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> ```
> // Went from 'rc' -> 'connected', but really we're storing the 'error':
> int connected = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> Could you follow up on each of these Evelina? Thank you!!
>
> I'll comment on the other Try cases in https://reviews.apache.org/r/28789/ :)
>
> Ben Mahler wrote:
> Disregard the "accept" case, I missed that we're actually storing the "accepted" socket so the existing name makes sense. :)
Ok, thanks for explaining me all this :D! I will create a patch for fixing all the coding style issues.
Should I make the following substitutions in the linux routing library?
sock -> s/socket
err -> error
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 85
> > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85>
> >
> > The pattern we use for succinctness and clarity in naming here is:
> >
> > ```
> > Try<int> connect = process::connect(...);
> > ```
> >
> > "try" is redundant here since it's encoded in the type.
> >
> > Ditto everywhere else. :)
>
> Evelina Dumitrescu wrote:
> See dhamon's last commit.
>
> Ben Mahler wrote:
> Ah thanks for pointing me to that! That looks better for sure, although I'm hoping to keep it even more consistent with how we've been writing things:
>
> ```
> Try<int> sock = process::socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
> ```
>
> I didn't expect to see any "socks" in our code. ;)
>
> Interestingly, looks like we leaked this pattern into the linux routing library:
>
> ```
> ? mesos git:(95752f8) ? grep -R 'sock ' src
> src/linux/routing/diagnosis/diagnosis.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(NETLINK_INET_DIAG);
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> src/linux/routing/route.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
> ? mesos git:(95752f8) ? grep -R 'sock ' 3rdparty
> ```
>
> So ideally one of these:
>
> ```
> Try<int> socket = proces::socket(...);
> Try<int> s = proces::socket(...);
> ```
>
> and more generally on the other callsites, in these cases "rc" should have been "error":
>
> ```
> // Went from 'rc' -> 'accepted', but really we're storing the 'error':
> int accepted = ::accept(s, (sockaddr*) &addr, &addrlen);
> // Change to:
> int error = ::accept(s, (sockaddr*) &addr, &addrlen);
> ```
>
> ```
> // Went from 'rc' -> 'bound', but really we're storing the 'error':
> int bound = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::bind(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> ```
> // Went from 'rc' -> 'connected', but really we're storing the 'error':
> int connected = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> // Change to:
> int error = ::connect(s, (sockaddr*) &addr, sizeof(addr));
> ```
>
> Could you follow up on each of these Evelina? Thank you!!
>
> I'll comment on the other Try cases in https://reviews.apache.org/r/28789/ :)
Disregard the "accept" case, I missed that we're actually storing the "accepted" socket so the existing name makes sense. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > Thanks Evelina and Dominic!
> >
> > I don't have the context on this change, but there are a few general code quality items that were missed in review :(
> >
> > Could you please follow up on them? Thank you!
Note that there should be a lot of "ditto" comments (e.g. try naming, braces, "s" as interface argument name, "rc" as a name) that I omitted here, try to address all the cases :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 40
> > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line40>
> >
> > Hm.. this comment is a bit odd, because it's on top of the first method, but not on top of the others.
> >
> > Mind removing it, or adding something more standalone (i.e. newline):
> >
> > ```
> > // Socket call wrappers for different address families.
> >
> > inline ...
> > ```
Thank you for pointing me that out! I submitted another review for this https://reviews.apache.org/r/28789/ .
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 50
> > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line50>
> >
> > What does 'rc' mean? :)
> >
> > Please just call it 'accept' or 'result', it's ok to hide the name in the former case.
Sorry for that. I was used with this convention from other projects(eg: linux kernel). It's the abreviation from return code. dhamon submitted a fix for that https://github.com/apache/mesos/commit/83d90df125f05ab3c4f1b31311f7e5f1c9d6f8c1.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, lines 51-52
> > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line51>
> >
> > Looks like the lack of braces here was missed.
See dhamon's last commit.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 85
> > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85>
> >
> > The pattern we use for succinctness and clarity in naming here is:
> >
> > ```
> > Try<int> connect = process::connect(...);
> > ```
> >
> > "try" is redundant here since it's encoded in the type.
> >
> > Ditto everywhere else. :)
See dhamon's last commit.
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, line 85
> > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85>
> >
> > The pattern we use for succinctness and clarity in naming here is:
> >
> > ```
> > Try<int> connect = process::connect(...);
> > ```
> >
> > "try" is redundant here since it's encoded in the type.
> >
> > Ditto everywhere else. :)
>
> Evelina Dumitrescu wrote:
> See dhamon's last commit.
Ah thanks for pointing me to that! That looks better for sure, although I'm hoping to keep it even more consistent with how we've been writing things:
```
Try<int> sock = process::socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
```
I didn't expect to see any "socks" in our code. ;)
Interestingly, looks like we leaked this pattern into the linux routing library:
```
? mesos git:(95752f8) ? grep -R 'sock ' src
src/linux/routing/diagnosis/diagnosis.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(NETLINK_INET_DIAG);
src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
src/linux/routing/route.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket();
? mesos git:(95752f8) ? grep -R 'sock ' 3rdparty
```
So ideally one of these:
```
Try<int> socket = proces::socket(...);
Try<int> s = proces::socket(...);
```
and more generally on the other callsites, in these cases "rc" should have been "error":
```
// Went from 'rc' -> 'accepted', but really we're storing the 'error':
int accepted = ::accept(s, (sockaddr*) &addr, &addrlen);
// Change to:
int error = ::accept(s, (sockaddr*) &addr, &addrlen);
```
```
// Went from 'rc' -> 'bound', but really we're storing the 'error':
int bound = ::bind(s, (sockaddr*) &addr, sizeof(addr));
// Change to:
int error = ::bind(s, (sockaddr*) &addr, sizeof(addr));
```
```
// Went from 'rc' -> 'connected', but really we're storing the 'error':
int connected = ::connect(s, (sockaddr*) &addr, sizeof(addr));
// Change to:
int error = ::connect(s, (sockaddr*) &addr, sizeof(addr));
```
Could you follow up on each of these Evelina? Thank you!!
I'll comment on the other Try cases in https://reviews.apache.org/r/28789/ :)
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 50
> > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line50>
> >
> > What does 'rc' mean? :)
> >
> > Please just call it 'accept' or 'result', it's ok to hide the name in the former case.
>
> Evelina Dumitrescu wrote:
> Sorry for that. I was used with this convention from other projects(eg: linux kernel). It's the abreviation from return code. dhamon submitted a fix for that https://github.com/apache/mesos/commit/83d90df125f05ab3c4f1b31311f7e5f1c9d6f8c1.
No apologies necessary, thanks for following up, it means a lot! :)
> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 40
> > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line40>
> >
> > Hm.. this comment is a bit odd, because it's on top of the first method, but not on top of the others.
> >
> > Mind removing it, or adding something more standalone (i.e. newline):
> >
> > ```
> > // Socket call wrappers for different address families.
> >
> > inline ...
> > ```
>
> Evelina Dumitrescu wrote:
> Thank you for pointing me that out! I submitted another review for this https://reviews.apache.org/r/28789/ .
Thank you!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64146
-----------------------------------------------------------
Thanks Evelina and Dominic!
I don't have the context on this change, but there are a few general code quality items that were missed in review :(
Could you please follow up on them? Thank you!
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106594>
Hm.. this comment is a bit odd, because it's on top of the first method, but not on top of the others.
Mind removing it, or adding something more standalone (i.e. newline):
```
// Socket call wrappers for different address families.
inline ...
```
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106595>
How about s/s/socket/ given this is an interface, and we'd like to express the meaning of 's' to the _caller_. In the other cases, we're calling a socket 's' in the _callee_, which is ok.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106593>
What does 'rc' mean? :)
Please just call it 'accept' or 'result', it's ok to hide the name in the former case.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106591>
Looks like the lack of braces here was missed.
3rdparty/libprocess/src/http.cpp
<https://reviews.apache.org/r/28716/#comment106596>
The pattern we use for succinctness and clarity in naming here is:
```
Try<int> connect = process::connect(...);
```
"try" is redundant here since it's encoded in the type.
Ditto everywhere else. :)
- Ben Mahler
On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:21 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2177
> https://issues.apache.org/jira/browse/MESOS-2177
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 9:21 p.m.)
Review request for mesos and Dominic Hamon.
Bugs: MESOS-2177
https://issues.apache.org/jira/browse/MESOS-2177
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 8:51 p.m.)
Review request for mesos and Dominic Hamon.
Changes
-------
Fixed the coding style issues.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review64074
-----------------------------------------------------------
Ship it!
i'll make these last changes and get this all committed.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106496>
colon and space
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106495>
s/on/to
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106494>
you need a space here, maybe a colon.
- Dominic Hamon
On Dec. 5, 2014, 12:31 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 12:31 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 8:31 p.m.)
Review request for mesos and Dominic Hamon.
Changes
-------
Removed the explicit std::string constructor and included the node in the error message for bind & connect methods.
Reversed the condition from if statement in the connect method from process.cpp.
Temporary dropped changes for 3rdparty/libprocess/src/net.hpp and 3rdparty/libprocess/src/httpd.cpp because this code is not been compiled at all.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 5:11 p.m.)
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 4, 2014, 11:28 p.m.)
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 4, 2014, 11:19 p.m.)
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/
-----------------------------------------------------------
(Updated Dec. 4, 2014, 11:06 p.m.)
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
Diffs (updated)
-----
3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
Diff: https://reviews.apache.org/r/28716/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 4, 2014, 8:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 66
> > <https://reviews.apache.org/r/28716/diff/1/?file=782735#file782735line66>
> >
> > there's no Error case here so this can be an int return. Alternatively, check the error code and return an ErrnoError.
I returned ErrnoError if the return code is < 0.
> On Dec. 4, 2014, 8:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 50
> > <https://reviews.apache.org/r/28716/diff/1/?file=782735#file782735line50>
> >
> > do you think it's worth checking the return and returning an Error directly?
Done that, thanks.
> On Dec. 4, 2014, 8:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/net.hpp, line 73
> > <https://reviews.apache.org/r/28716/diff/1/?file=782738#file782738line73>
> >
> > if process::bind returns a Try<int> you can check IsError instead and throw with the Error string as a message.
Made bind return ErrnoError and checked with isError().
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review63899
-----------------------------------------------------------
On Dec. 4, 2014, 11:19 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2014, 11:19 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
> 3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28716: Created accept, bind,
connect and getsockname wrappers for different protocol families
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28716/#review63899
-----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106213>
do you think it's worth checking the return and returning an Error directly?
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106212>
use stringify() because to_string is not available (i think) in g++-4.4.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106214>
there's no Error case here so this can be an int return. Alternatively, check the error code and return an ErrnoError.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106215>
see comment for 'bind' above.
3rdparty/libprocess/include/process/socket.hpp
<https://reviews.apache.org/r/28716/#comment106216>
stringify
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106218>
if process::bind returns a Try<int> you can check IsError instead and throw with the Error string as a message.
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106220>
you don't need this to be outside the do/while.
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106219>
else on this line, or no else at all given that the 'if' throws.
3rdparty/libprocess/src/net.hpp
<https://reviews.apache.org/r/28716/#comment106221>
these two conditions can be combined.
you can use the error returned in tryAccept in the runtime_error.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/28716/#comment106222>
if connect returns an error this can be made simpler as the Failure will just return the Error as returned by connect.
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/28716/#comment106223>
return nodeGetSockName;
no need to create a new Error if you know you have a Try<Node> that is an Error :)
in fact, this whole block can be replaced by
return process::getsockname(get(), AF_INET);
3rdparty/libprocess/src/process.cpp
<https://reviews.apache.org/r/28716/#comment106225>
why would the return be < 0 if tryAccept is not an error?
3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/28716/#comment106226>
space before '='
- Dominic Hamon
On Dec. 4, 2014, 12:14 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28716/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2014, 12:14 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Created accept, bind, connect and getsockname wrappers in socket.hpp for different protocol families
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/socket.hpp ab080c154095029c4a01d189a5fd8a178ba6c92e
> 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358
> 3rdparty/libprocess/src/httpd.cpp 902ba89b18c5d7edf68ca9d17c55e5727529f96e
> 3rdparty/libprocess/src/net.hpp 7bf6085e1bd9ffa0e42a5da9c3567521ff4c0713
> 3rdparty/libprocess/src/process.cpp 4db7d56af710577e08a9d7dbeb92a1f01559401f
> 3rdparty/libprocess/src/tests/http_tests.cpp a90e65f77904da0a45e1cc0cc9889ae69354a1a5
> 3rdparty/libprocess/src/tests/process_tests.cpp dec62e88ec993433e1a0777593bb2657b43636dc
>
> Diff: https://reviews.apache.org/r/28716/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>