[login_manager] Make hang-detection flag optionally take an ... [chromiumos/platform/login_manager : master]

34 views
Skip to first unread message

Chris Masone (Code Review)

unread,
Nov 19, 2012, 7:59:35 PM11/19/12
to
Chris Masone has uploaded a new change for review.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................

[login_manager] Make hang-detection flag optionally take an interval

By allowing this flag to optionally take a value, the interval
can be shortened for testing, or set to the default, while
still allowing the feature to be disabled completely.

BUG=chromium-os:35729
TEST=unit

Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
---
M liveness_checker_impl.cc
M liveness_checker_impl.h
M liveness_checker_impl_unittest.cc
M session_manager_main.cc
M session_manager_service.cc
M session_manager_service.h
6 files changed, 42 insertions(+), 21 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/platform/login_manager refs/changes/46/38346/1
--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 19, 2012, 8:03:42 PM11/19/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 1: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>
Gerrit-Reviewer: Chris Masone <cma...@chromium.org>
Gerrit-Reviewer: Daniel Erat <de...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>

Daniel Erat (Code Review)

unread,
Nov 19, 2012, 8:12:56 PM11/19/12
to Chris Masone, Gerrit
Daniel Erat has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File liveness_checker_impl.cc
Line 37: enable_aborting_(interval_seconds >= 0),
this feels too tricky. i think it'd be better to keep separate interval and "enabled" settings.

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 19, 2012, 8:29:12 PM11/19/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File liveness_checker_impl.cc
Line 37: enable_aborting_(interval_seconds >= 0),
Done

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 19, 2012, 8:30:09 PM11/19/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 2: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Daniel Erat (Code Review)

unread,
Nov 19, 2012, 9:46:18 PM11/19/12
to Chris Masone, Gerrit
Daniel Erat has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 2: (4 inline comments)

....................................................
File liveness_checker_impl.cc
Line 38: kLivenessCheckIntervalSeconds),
i think it'd also be cleaner to get rid of the "negative means use some default" behavior and just make the caller pass in whatever interval they want. (otherwise, you end up with e.g. session_manager_main.cc claiming that the default is 60, when the actual constant lives here -- it'd be easy for the usage message to get out of sync.)

....................................................
File liveness_checker_impl_unittest.cc
Line 108: false, /* Disable aborting */
nit: i think that // is usually used for end-of-line comments; c-style comments are used for e.g.

false /* disable aborting */,

....................................................
File session_manager_main.cc
Line 135: hang_detection_interval = -1;
you're assigning -1 to an unsigned int here

....................................................
File session_manager_service.h
Line 74: uint hang_detection_interval,
document that a negative interval disables hang detection; also make this be an int instead of an unsigned int.

it'd be good to put a "_sec" prefix on the name so that callers aren't left wondering if it's ms, seconds, etc. base::TimeDelta would be best, although that makes it tougher to make this double as a enable/disable setting -- you could use null/0 for that, i suppose.

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 20, 2012, 11:57:59 AM11/20/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 2: (4 inline comments)

....................................................
File liveness_checker_impl.cc
Line 38: kLivenessCheckIntervalSeconds),
it's all gone now, the -1 stuff

....................................................
File liveness_checker_impl_unittest.cc
Line 108: false, /* Disable aborting */
Done

....................................................
File session_manager_main.cc
Line 135: hang_detection_interval = -1;
hurr.

The intent was to go back to uints all over and get rid of the -1 stuff, but I only got halfway there. Shouldn't send code at the end of the day :-/

....................................................
File session_manager_service.h
Line 74: uint hang_detection_interval,
making it a timedelta, and adding explicit bool

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 20, 2012, 11:58:06 AM11/20/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 4: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Daniel Erat (Code Review)

unread,
Nov 20, 2012, 12:01:30 PM11/20/12
to Chris Masone, Gerrit
Daniel Erat has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File session_manager_main.cc
Line 138: if (base::StringToUint(flag, &from_flag)) {
nit: can you just do this, or are you worried about StringToUint changing the passed-in dest even on parse failure?

if (!base::StringToUint(flag, &hang_detection_interval))
DLOG(WARNING) << "etc.";

(if you're worried about this, it'd still be a bit shorter to reassign the default to hang_detection_interval in the failure case)

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Daniel Erat (Code Review)

unread,
Nov 20, 2012, 12:01:58 PM11/20/12
to Chris Masone, Gerrit
Daniel Erat has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 4: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>

Chris Masone (Code Review)

unread,
Nov 20, 2012, 1:00:57 PM11/20/12
to Gerrit, Daniel Erat
Chris Masone has posted comments on this change.

Change subject: [login_manager] Make hang-detection flag optionally take an interval
......................................................................


Patch Set 4: Ready

--
To view, visit https://gerrit.chromium.org/gerrit/38346
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76e896be631cbbb9012f3f3e2b4eba94f4a20633
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/login_manager
Gerrit-Branch: master
Gerrit-Owner: Chris Masone <cma...@chromium.org>
Reply all
Reply to author
Forward
0 new messages