[debug_server] Allow configuring a default action on program crashes

[anevilyak]

In the interest of things like automated testsuites, it would be nice if debug_server could be configured to automatically take an action on crashes rather than prompting the user, e.g. silently terminate, or save a crash report to a predefined location without a popup. This could be configured via a simple driver_settings-style file, perhaps with a preflet as well.

It may additionally be nice to be able to override the default action and force a different one for a specific team, but that would take a bit more work, as that would require both a corresponding API call, as well as some work on the debug_server side to be able to track such per-team overrides.

[bonefish]

A driver settings file sounds like a simple enough solution. It could not only specify the general action -- "user", "kill", "log", "debug" -- but also executable specific ones, possibly even supporting simple pattern matching, e.g.:

default_action     user
executable_actions {
  app_server log
  input_server log
  registrar log
  /boot/home/develop/my_test_suite/*/test-* log
}

I don't think a preflet is needed, as it seems to be functionality only developers need in rare situations.

The feature to set the action for a team via an API call (and via environment variable) would still be nice.

[anevilyak]

Replying to bonefish:

A driver settings file sounds like a simple enough solution. It could not only specify the general action -- "user", "kill", "log", "debug" -- but also executable specific ones, possibly even supporting simple pattern matching, e.g.: [...]

Sounds good.

The feature to set the action for a team via an API call (and via environment variable) would still be nice.

The only issue I see with the environment variable version is how debug_server would detect it. At least e.g. team_info doesn't provide access to the target team's environment that I can see, so the only means by which I could see that working would be for some init function in libroot to detect that and make the corresponding API call on the app's behalf. Which init function that could be, I'm uncertain of right now though.

[bonefish]

Yes, the environment variable would have to be checked by some init code that calls the API function accordingly. I'd probably do that in src/system/libroot/libroot_init.c directly after __init_env() has been called.

Given that we'll probably introduce additional environment variables for various purposes in the future, it might be a good idea to eventually have a single place (maybe as early as the ​runtime loader entry function) that iterates once through the environment and gets the values of all interesting ones, thus avoiding having dozens of (not particularly efficient) getenv() calls all over the place. ATM that isn't particularly relevant, though.

[anevilyak]

Will see what I can come up with over the next few days.

[anevilyak]

Initial implementation of the settings file portion attached. Hasn't yet been heavily tested with respect to the pattern matching part, but the basics should work. The discussed API for overriding dynamically isn't yet included/implemented, nor are the environment variables, will take a bit more time. Review/comments welcome.

[bonefish]

I only had a quick look. A few comments:

• I would read the settings whenever a team crashes. This allows changing the settings at run-time without having to restart the debug server.
• static int32 action_for_string(): Missing line break.
• The code in Init() relies on the "default_action" setting being specified before any "executable_actions" (fDefaultDebugAction is used for the latter).
• It probably doesn't make much of a difference in this case, but there's a private C++ API for driver settings access, which may be more convenient to use (headers/private/storage/DriverSettings.h).
• I don't think support for case insensitive settings is needed.

[anevilyak]

Replying to bonefish:

• I would read the settings whenever a team crashes. This allows changing the settings at run-time without having to restart the debug server.

Good idea, I suppose in the general case there will only be a small number of entries so this shouldn't be too much overhead.

• static int32 action_for_string(): Missing line break.

Will fix.

• The code in Init() relies on the "default_action" setting being specified before any "executable_actions" (fDefaultDebugAction is used for the latter).

fDefaultDebugAction is initialized in the constructor to be the prompt action. action_for_string() only uses it as a fallback in case the requested action didn't match any of the recognized strings, so the only effect in the above scenario is that an entry like StyledEdit dbeug or some other similar typo would simply fall back to prompting, rather than the new default action specified later in the file. I suppose I could get around that by requesting that one explicitly up front via the normal settings API rather than by tree walking, but I wanted to avoid having to make multiple passes of the tree unnecessarily, especially if we follow the above suggestion and constantly evaluate it on the fly (thinking specifically of the testsuite scenario where we might potentially get called hundreds of times in short order). Thoughts?

• It probably doesn't make much of a difference in this case, but there's a private C++ API for driver settings access, which may be more convenient to use (headers/private/storage/DriverSettings.h).

Wasn't aware of that, will look into it.

• I don't think support for case insensitive settings is needed.

Noted. Thanks for the review, will work on some improvements as time permits.

[bonefish]

Replying to anevilyak:

I suppose I could get around that by requesting that one explicitly up front via the normal settings API rather than by tree walking, but I wanted to avoid having to make multiple passes of the tree unnecessarily, especially if we follow the above suggestion and constantly evaluate it on the fly (thinking specifically of the testsuite scenario where we might potentially get called hundreds of times in short order). Thoughts?

I think compared to reading and parsing the settings file, iterating a second time through the settings wouldn't have much impact. That being said, action_for_string() could simply propagate the error back instead of being passed a default value (bool get_action_for_string(const char*, int32&)). This way the caller can handle a broken entry as appropriate (e.g. ignore it).

[tangobravo]

Replying to bonefish:

• I would read the settings whenever a team crashes. This allows changing the settings at run-time without having to restart the debug server.

Isn't node monitoring the usual solution to allow run-time updates without having to constantly re-parse the (probably unchanged) settings file?

[bonefish]

Replying to tangobravo:

Isn't node monitoring the usual solution to allow run-time updates without having to constantly re-parse the (probably unchanged) settings file?

Given that currently the settings aren't needed until some team actually crashes (hopefully a rare occasion :-)), there's little point in detecting changes live. It would just complicate things. Moreover, application crashes aren't really time critical situations and in the overall process reading the settings file is probably negligible, anyway.

[anevilyak]

Settings file implemented with adjustments based on feedback in hrev46547. Leaving ticket open until the discussed API and environment variables have also been implemented.