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 2017/02/09 08:31:44 UTC
[kudu-CR] tls socket: fix handling of syscall errors
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5954
to review the following change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
tls_socket: fix handling of syscall errors
This prevents a case where we'd see an error like 'TlsSocket::Recv: Success'
when trying to read from a TLS socket which got abruptly shut down by the
remote side.
Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
---
M src/kudu/security/tls_socket.cc
1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/5954/1
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5954
to look at the new patch set (#2).
Change subject: tls_socket: fix handling of syscall errors
......................................................................
tls_socket: fix handling of syscall errors
This prevents a case where we'd see an error like 'TlsSocket::Recv: Success'
when trying to read from a TLS socket which got abruptly shut down by the
remote side.
Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
---
M src/kudu/security/tls_socket.cc
1 file changed, 25 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/5954/2
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
Patch Set 1: Code-Review+1
LGTM when the other comment is addressed.
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5954/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 127: if (error_code == SSL_ERROR_SYSCALL && ERR_peek_error() == 0) {
Doesn't this have some overlap with GetSSLErrorDescription() in security/openssl_util.cc?
Anyway we could use that to convey the same errors?
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5954
to look at the new patch set (#3).
Change subject: tls_socket: fix handling of syscall errors
......................................................................
tls_socket: fix handling of syscall errors
This prevents a case where we'd see an error like 'TlsSocket::Recv: Success'
when trying to read from a TLS socket which got abruptly shut down by the
remote side.
Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
---
M src/kudu/security/tls_socket.cc
1 file changed, 26 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/5954/3
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5954/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 127: if (error_code == SSL_ERROR_SYSCALL && ERR_peek_error() == 0) {
> Doesn't this have some overlap with GetSSLErrorDescription() in security/op
I don't think it can really be consolidated, because we also are relying on the 'bytes_read' value to stringify it here. The docs for this particular error code seem to be specific to SSL_read() rather than a generic way to handle the code.
Line 137: return Status::NetworkError("connection reset by peer", "", ECONNRESET);
> May want to make this match the others, or have the others match this. e.g
k, I'll change them to try to be more consistent and just try to distinguish SSL-specific errors from more generic socket errors.
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5954/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 137: return Status::NetworkError("connection reset by peer", "", ECONNRESET);
May want to make this match the others, or have the others match this. e.g.
Status::NetworkError("TlsSocket::Recv", "received connection reset", ECONNRESET)
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
tls_socket: fix handling of syscall errors
This prevents a case where we'd see an error like 'TlsSocket::Recv: Success'
when trying to read from a TLS socket which got abruptly shut down by the
remote side.
Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Reviewed-on: http://gerrit.cloudera.org:8080/5954
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/security/tls_socket.cc
1 file changed, 26 insertions(+), 3 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tls socket: fix handling of syscall errors
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: tls_socket: fix handling of syscall errors
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5954
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a0a63f861d71bd3186567ff98148476795530ab
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No