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 2016/11/10 00:51:35 UTC
[kudu-CR] Check sanity of standard file descriptors when starting daemons
Hello Jean-Daniel Cryans, Adar Dembo, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5030
to review the following change.
Change subject: Check sanity of standard file descriptors when starting daemons
......................................................................
Check sanity of standard file descriptors when starting daemons
This adds a sanity check at daemon startup that the standard file
descriptors are open. If they are closed at startup, then those numeric
FDs will be reused by other open files later, and it's possible that a
library trying to write to stderr accidentally writes to some other open
file (such as our data!)
Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
---
M src/kudu/util/init.cc
1 file changed, 29 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/5030/1
--
To view, visit http://gerrit.cloudera.org:8080/5030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] Check sanity of standard file descriptors when starting daemons
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Check sanity of standard file descriptors when starting daemons
......................................................................
Patch Set 1: Code-Review+1
All java tests pass on macOS with this patch, as they should.
--
To view, visit http://gerrit.cloudera.org:8080/5030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Check sanity of standard file descriptors when starting daemons
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Check sanity of standard file descriptors when starting daemons
......................................................................
Check sanity of standard file descriptors when starting daemons
This adds a sanity check at daemon startup that the standard file
descriptors are open. If they are closed at startup, then those numeric
FDs will be reused by other open files later, and it's possible that a
library trying to write to stderr accidentally writes to some other open
file (such as our data!)
Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Reviewed-on: http://gerrit.cloudera.org:8080/5030
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/util/init.cc
1 file changed, 29 insertions(+), 0 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Check sanity of standard file descriptors when starting daemons
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Check sanity of standard file descriptors when starting daemons
......................................................................
Patch Set 1:
I suppose a death test would be possible, but it's such a rare scenario I don't think it's worth it (and unlikely to regress)
--
To view, visit http://gerrit.cloudera.org:8080/5030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Check sanity of standard file descriptors when starting daemons
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Check sanity of standard file descriptors when starting daemons
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Ah, I was hoping this could be tested with a death test. Not possible?
http://gerrit.cloudera.org:8080/#/c/5030/1/src/kudu/util/init.cc
File src/kudu/util/init.cc:
PS1, Line 60: This is a rare enough issue that people can deal with
: // the core dump.
If this turns out to happen more commonly, I suggest we reopen the missing fds from /dev/null instead. We'll see how it goes.
--
To view, visit http://gerrit.cloudera.org:8080/5030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie57efbb63c588e39ac70777ba7b21496aae7fc27
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes