You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/05/01 05:08:53 UTC
[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15706 )
Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.h@111
PS6, Line 111: std::vector<int> TakeOutboundFds() {
nit: not add &&?
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.cc@148
PS6, Line 148: move
nit: hrm, i'm surprised this works without using std::. Is this perhaps inherited from boost? Or am I missing a using std:: somewhere?
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/reactor.h@201
PS6, Line 201: pid_for_tests
nit: tid, not pid?
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_controller.cc@167
PS6, Line 167: SendFileDescriptor
nit: given this isn't actually sending the descriptor yet, consider AddFileDescriptor for parity with AddOutboundSidecar?
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@447
PS6, Line 447:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@472
PS6, Line 472: int aux_len = CMSG_LEN(send_fds.size() * sizeof(int));
Why don't we need to check that send_fds doesn't exceed kMaxPassedFds?
http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@587
PS6, Line 587: if (cmsg->cmsg_level == SOL_SOCKET &&
: cmsg->cmsg_type == SCM_RIGHTS) {
: if (cmsg->cmsg_len <= sizeof(struct cmsghdr)) {
: // An invalid or empty SCM_RIGHTS.
: continue;
: }
Do you think it's worth vlogging skipped cmgs?
--
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 01 May 2020 05:08:53 +0000
Gerrit-HasComments: Yes