[kudu-CR] net: make Sockaddr more generic for other address families
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. net: make Sockaddr more generic for other address families This paves the way for supporting other socket address types: namely IPv6 and Unix domain sockets. This patch doesn't add any new support, but increases the size of Sockaddr appropriately and making the methods more generic. The approach here is to use a union inside of the Sockaddr class which could store an address of any type. An alternative (taken by a long-ago attempt at unix sockets) would be to make Sockaddr an abstract base class with derived classes for each socket type. I went with this approach since all of the POSIX socket APIs expect this "tagged union" approach -- the same APIs are used whether we want to talk to a unix socket or an IPv4. The downside of this approach is a slightly larger Sockaddr object footprint, but we shouldn't have a lot of these on the heap. One functional change: the default constructor for Sockaddr now creates an "uninitialized" instance, whereas it used to create a wildcard address. That's now an explicit static method. Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Reviewed-on: http://gerrit.cloudera.org:8080/15690 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong Reviewed-by: Alexey Serbin --- M src/kudu/clock/builtin_ntp.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/webserver.cc M src/kudu/tserver/scanners.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.cc 13 files changed, 217 insertions(+), 85 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] net: make Sockaddr more generic for other address families
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Tue, 14 Apr 2020 21:56:41 + Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Tue, 14 Apr 2020 21:47:00 + Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15690 to look at the new patch set (#4). Change subject: net: make Sockaddr more generic for other address families .. net: make Sockaddr more generic for other address families This paves the way for supporting other socket address types: namely IPv6 and Unix domain sockets. This patch doesn't add any new support, but increases the size of Sockaddr appropriately and making the methods more generic. The approach here is to use a union inside of the Sockaddr class which could store an address of any type. An alternative (taken by a long-ago attempt at unix sockets) would be to make Sockaddr an abstract base class with derived classes for each socket type. I went with this approach since all of the POSIX socket APIs expect this "tagged union" approach -- the same APIs are used whether we want to talk to a unix socket or an IPv4. The downside of this approach is a slightly larger Sockaddr object footprint, but we shouldn't have a lot of these on the heap. One functional change: the default constructor for Sockaddr now creates an "uninitialized" instance, whereas it used to create a wildcard address. That's now an explicit static method. Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 --- M src/kudu/clock/builtin_ntp.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/webserver.cc M src/kudu/tserver/scanners.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.cc 13 files changed, 217 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/15690/4 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] net: make Sockaddr more generic for other address families
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/15690 )
Change subject: net: make Sockaddr more generic for other address families
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/net_util-test.cc
File src/kudu/util/net/net_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/net_util-test.cc@232
PS3, Line 232: ASSERT_TRUE(uninitialized_1 == uninitialized_2);
:
: Sockaddr wildcard = Sockaddr::Wildcard();
: ASSERT_FALSE(wildcard == uninitialized_1);
: ASSERT_FALSE(uninitialized_1 == wildcard);
:
: Sockaddr wildcard_2 = Sockaddr::Wildcard();
: ASSERT_TRUE(wildcard == wildcard_2);
: ASSERT_TRUE(wildcard_2 == wildcard);
:
: Sockaddr ip_port;
: ASSERT_OK(ip_port.ParseString("127.0.0.1:12345", 0));
: ASSERT_FALSE(ip_port == uninitialized_1);
: ASSERT_FALSE(ip_port == wildcard);
: ASSERT_TRUE(ip_port == ip_port);
> nit: any reason to not use ASSERT_EQ and ASSERT_NE for these?
that requires defining either an operator<< or some kind of gtest printer
method for Sockaddr, and I was too lazy to do that
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/sockaddr.h@134
PS3, Line 134: // Set the type of the internal storage to 'family' and adjust
ASAN poisoning
: // appropriately.
> nit: meant for another method?
Ack
--
To view, visit http://gerrit.cloudera.org:8080/15690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3
Gerrit-Change-Number: 15690
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Tue, 14 Apr 2020 16:13:32 +
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15690 )
Change subject: net: make Sockaddr more generic for other address families
..
Patch Set 3: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/net_util-test.cc
File src/kudu/util/net/net_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/net_util-test.cc@232
PS3, Line 232: ASSERT_TRUE(uninitialized_1 == uninitialized_2);
:
: Sockaddr wildcard = Sockaddr::Wildcard();
: ASSERT_FALSE(wildcard == uninitialized_1);
: ASSERT_FALSE(uninitialized_1 == wildcard);
:
: Sockaddr wildcard_2 = Sockaddr::Wildcard();
: ASSERT_TRUE(wildcard == wildcard_2);
: ASSERT_TRUE(wildcard_2 == wildcard);
:
: Sockaddr ip_port;
: ASSERT_OK(ip_port.ParseString("127.0.0.1:12345", 0));
: ASSERT_FALSE(ip_port == uninitialized_1);
: ASSERT_FALSE(ip_port == wildcard);
: ASSERT_TRUE(ip_port == ip_port);
nit: any reason to not use ASSERT_EQ and ASSERT_NE for these?
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15690/3/src/kudu/util/net/sockaddr.h@134
PS3, Line 134: // Set the type of the internal storage to 'family' and adjust
ASAN poisoning
: // appropriately.
nit: meant for another method?
--
To view, visit http://gerrit.cloudera.org:8080/15690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3
Gerrit-Change-Number: 15690
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Sat, 11 Apr 2020 19:32:51 +
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@62 PS2, Line 62: struct sockaddr_in addr; : memset(, 0, sizeof(addr)); : addr.sin_family = AF_INET; : addr.sin_addr.s_addr = INADDR_ANY; : return Sockaddr(addr); > was curious so I tried this on godbolt: https://godbolt.org/z/xC8qqF Ack -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Fri, 10 Apr 2020 18:59:09 + Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Fri, 10 Apr 2020 18:03:20 + Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15690 to look at the new patch set (#3). Change subject: net: make Sockaddr more generic for other address families .. net: make Sockaddr more generic for other address families This paves the way for supporting other socket address types: namely IPv6 and Unix domain sockets. This patch doesn't add any new support, but increases the size of Sockaddr appropriately and making the methods more generic. The approach here is to use a union inside of the Sockaddr class which could store an address of any type. An alternative (taken by a long-ago attempt at unix sockets) would be to make Sockaddr an abstract base class with derived classes for each socket type. I went with this approach since all of the POSIX socket APIs expect this "tagged union" approach -- the same APIs are used whether we want to talk to a unix socket or an IPv4. The downside of this approach is a slightly larger Sockaddr object footprint, but we shouldn't have a lot of these on the heap. One functional change: the default constructor for Sockaddr now creates an "uninitialized" instance, whereas it used to create a wildcard address. That's now an explicit static method. Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 --- M src/kudu/clock/builtin_ntp.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/webserver.cc M src/kudu/tserver/scanners.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.cc 13 files changed, 217 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/15690/3 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Volodymyr Verovkin
[kudu-CR] net: make Sockaddr more generic for other address families
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/15690 )
Change subject: net: make Sockaddr more generic for other address families
..
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/15690/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15690/2//COMMIT_MSG@10
PS2, Line 10: Unix domain sockets
> Curious about the motivation of this change.
yea, was trying to prototype some unix domain socket based transport,
potentially involving passing a memfd to do shared memory. Tis was something we
tried many years ago (via an intern) but never got working great. Figured I'd
give it another go.
This will also help Facebook who needs ipv6 support.
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/rpc/rpc-test-base.h@606
PS2, Line 606: Status StartFakeServer(Socket *listen_sock, Sockaddr
*listen_addr) {
> warning: method 'StartFakeServer' can be made static [readability-convert-m
Done
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@39
PS2, Line 39: // Create an uninitialized socket. This instance must be
assigned to before usage.
: Sockaddr();
> Can the default constructor be deleted instead?
It's needed for APIs like:
Sockaddr addr;
CHECK_OK(sock.GetPeerAddress());
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@59
PS2, Line 59: // Compare two addresses bytewise.
: static bool BytewiseLess(const Sockaddr& a, const Sockaddr& b);
:
: // Return the IPv4 wildcard address.
: static Sockaddr Wildcard();
> style nit: move these two static methods up to be in the very beginning of
i'll go halfway here -- I think the Wildcard() "static factory" makes sense to
be up near the constructor, but BytewiseLess probably belongs near operator==
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@69
PS2, Line 69: Parse a string IP address
> From the implementation this function only parses IPV4 addresses. Comment s
Done
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@81
PS2, Line 81: int
> nit: while you are here, maybe change this to be uint16_t ?
the google style guide recommends sticking to 'int' unless trying to represent
bit patterns, etc. I'll add an assertion, though
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@96
PS2, Line 96: return storage_.generic.ss_family;
> Should there be a check for is_initialized()?
Done
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@131
PS2, Line 131: union {
> Clearly sockaddr_storage was designed to be used in this way but I never kn
yea I went halfway down the road of making my own version of it, and then
discovered it in some man page somewhere
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@62
PS2, Line 62: struct sockaddr_in addr;
: memset(, 0, sizeof(addr));
: addr.sin_family = AF_INET;
: addr.sin_addr.s_addr = INADDR_ANY;
: return Sockaddr(addr);
> Alternatively a static const object could be created just once and a copy s
was curious so I tried this on godbolt: https://godbolt.org/z/xC8qqF
It looks like the "copy from static" is slightly shorter on the hot path but
adds a branch for the static initialization which makes the whole function
larger (and less inlinable). The "construct on the fly" as seen here is
relatively concise -- it elides the extra memcpy etc and just sets up the
result object "in place".
Given this isn't a hot path anyway I think I prefer this option - fewer lines
of code and more straightforward to read. I just did the above test because I
was curios which optimizations would apply.
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@108
PS2, Line 108: addrlen
> Might it be the case that other.addrlen() != addrlen() ?
nice catch!
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@173
PS2, Line 173:
> style nit: remove the extra space
Done
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@173
PS2, Line 173: const_cast
> I might be missing something, but it seems const_cast is not needed?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3
Gerrit-Change-Number: 15690
Gerrit-PatchSet: 2
[kudu-CR] net: make Sockaddr more generic for other address families
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@59 PS2, Line 59: // Compare two addresses bytewise. : static bool BytewiseLess(const Sockaddr& a, const Sockaddr& b); : : // Return the IPv4 wildcard address. : static Sockaddr Wildcard(); style nit: move these two static methods up to be in the very beginning of the method list? http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@81 PS2, Line 81: int nit: while you are here, maybe change this to be uint16_t ? http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@85 PS2, Line 85: int ditto http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@108 PS2, Line 108: addrlen Might it be the case that other.addrlen() != addrlen() ? http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@173 PS2, Line 173: ditto http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@173 PS2, Line 173: style nit: remove the extra space http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@173 PS2, Line 173: const_cast I might be missing something, but it seems const_cast is not needed? http://man7.org/linux/man-pages/man3/getnameinfo.3.html -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Thu, 09 Apr 2020 04:45:22 + Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/15690 )
Change subject: net: make Sockaddr more generic for other address families
..
Patch Set 2: Code-Review+2
(1 comment)
Looks like a step in the right direction for this API.
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h
File src/kudu/util/net/sockaddr.h:
http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@131
PS2, Line 131: union {
Clearly sockaddr_storage was designed to be used in this way but I never knew
about this. Very nice.
--
To view, visit http://gerrit.cloudera.org:8080/15690
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3
Gerrit-Change-Number: 15690
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:58:07 +
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Comment-Date: Thu, 09 Apr 2020 00:19:36 + Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15690/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15690/2//COMMIT_MSG@10 PS2, Line 10: Unix domain sockets Curious about the motivation of this change. Plan to use Unix domain sockets instead of network for IPC on same host for better performance? -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Apr 2020 23:41:54 + Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15690 ) Change subject: net: make Sockaddr more generic for other address families .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h File src/kudu/util/net/sockaddr.h: http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@39 PS2, Line 39: // Create an uninitialized socket. This instance must be assigned to before usage. : Sockaddr(); Can the default constructor be deleted instead? http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@69 PS2, Line 69: Parse a string IP address >From the implementation this function only parses IPV4 addresses. Comment >should be updated. http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.h@96 PS2, Line 96: return storage_.generic.ss_family; Should there be a check for is_initialized()? http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: http://gerrit.cloudera.org:8080/#/c/15690/2/src/kudu/util/net/sockaddr.cc@62 PS2, Line 62: struct sockaddr_in addr; : memset(, 0, sizeof(addr)); : addr.sin_family = AF_INET; : addr.sin_addr.s_addr = INADDR_ANY; : return Sockaddr(addr); Alternatively a static const object could be created just once and a copy simply returned. -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Apr 2020 23:32:42 + Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15690 to look at the new patch set (#2). Change subject: net: make Sockaddr more generic for other address families .. net: make Sockaddr more generic for other address families This paves the way for supporting other socket address types: namely IPv6 and Unix domain sockets. This patch doesn't add any new support, but increases the size of Sockaddr appropriately and making the methods more generic. The approach here is to use a union inside of the Sockaddr class which could store an address of any type. An alternative (taken by a long-ago attempt at unix sockets) would be to make Sockaddr an abstract base class with derived classes for each socket type. I went with this approach since all of the POSIX socket APIs expect this "tagged union" approach -- the same APIs are used whether we want to talk to a unix socket or an IPv4. The downside of this approach is a slightly larger Sockaddr object footprint, but we shouldn't have a lot of these on the heap. One functional change: the default constructor for Sockaddr now creates an "uninitialized" instance, whereas it used to create a wildcard address. That's now an explicit static method. Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 --- M src/kudu/clock/builtin_ntp.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/webserver.cc M src/kudu/tserver/scanners.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.cc 13 files changed, 188 insertions(+), 81 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/15690/2 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] net: make Sockaddr more generic for other address families
Hello Mike Percy, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15690 to review the following change. Change subject: net: make Sockaddr more generic for other address families .. net: make Sockaddr more generic for other address families This paves the way for supporting other socket address types: namely IPv6 and Unix domain sockets. This patch doesn't add any new support, but increases the size of Sockaddr appropriately and making the methods more generic. The approach here is to use a union inside of the Sockaddr class which could store an address of any type. An alternative (taken by a long-ago attempt at unix sockets) would be to make Sockaddr an abstract base class with derived classes for each socket type. I went with this approach since all of the POSIX socket APIs expect this "tagged union" approach -- the same APIs are used whether we want to talk to a unix socket or an IPv4. The downside of this approach is a slightly larger Sockaddr object footprint, but we shouldn't have a lot of these on the heap. One functional change: the default constructor for Sockaddr now creates an "uninitialized" instance, whereas it used to create a wildcard address. That's now an explicit static method. Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 --- M src/kudu/clock/builtin_ntp.cc M src/kudu/rpc/mt-rpc-test.cc M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/proxy.cc M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/webserver.cc M src/kudu/tserver/scanners.cc M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/sockaddr.h M src/kudu/util/net/socket.cc 13 files changed, 179 insertions(+), 81 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/15690/1 -- To view, visit http://gerrit.cloudera.org:8080/15690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ifff57e13a0e9b86b191d776ead67371b1e2ed4e3 Gerrit-Change-Number: 15690 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy
