You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/04/08 21:58:17 UTC
[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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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<sockaddr *>
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 04:45:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 11 Apr 2020 19:32:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 16:13:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 18:03:20 +0000
Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 21:47:00 +0000
Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Apr 2020 21:56:41 +0000
Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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(&addr, 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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 23:32:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:58:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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(&addr, 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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Apr 2020 18:59:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 00:19:36 +0000
Gerrit-HasComments: No
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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(&addr));
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(&addr, 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<sockaddr *>
> 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
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 20:56:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] net: make Sockaddr more generic for other address families
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Apr 2020 23:41:54 +0000
Gerrit-HasComments: Yes