On 07/31/2012 11:40 AM, Jakub Filak wrote:
* naive implementation of running of event chain in report-cli
Signed-off-by: Jakub Filak <jfilak(a)redhat.com>
---
src/cli/cli-report.c | 223 ++++++++++++++++++++++++++++++++++++++------------
src/cli/cli-report.h | 1 +
src/cli/cli.c | 34 ++++----
3 files changed, 187 insertions(+), 71 deletions(-)
diff --git a/src/cli/cli-report.c b/src/cli/cli-report.c
index 988bfc7..e204435 100644
--- a/src/cli/cli-report.c
+++ b/src/cli/cli-report.c
@@ -520,10 +520,30 @@ static char *do_log_and_save_line(char *log_line, void *param)
l_state->last_line = log_line;
return NULL;
}
+
+static int run_event(struct run_event_state *state, const char *dump_dir_name,
+ const char *event, struct logging_state *l_state)
+{
+ // Export overridden settings as environment variables
+ GList *env_list = export_event_config(event);
+
+ int r = run_event_on_dir_name(state, dump_dir_name, event);
+ if (r == 0 && state->children_count == 0)
+ {
+ l_state->last_line = xasprintf("Error: no processing is specified for
event '%s'",
+ event);
+ r = -1;
+ }
+
+ // Unexport overridden settings
+ unexport_event_config(env_list);
+
+ return r;
+}
+
static int run_events(const char *dump_dir_name, GList *events, const char *event_type)
{
int error_cnt = 0;
- GList *env_list = NULL;
// Run events
struct logging_state l_state;
@@ -534,18 +554,7 @@ static int run_events(const char *dump_dir_name, GList *events,
const char *even
for (GList *li = events; li; li = li->next)
{
char *event = (char *) li->data;
-
- // Export overridden settings as environment variables
- env_list = export_event_config(event);
-
- int r = run_event_on_dir_name(run_state, dump_dir_name, event);
- if (r == 0 && run_state->children_count == 0)
- {
- l_state.last_line = xasprintf("Error: no processing is specified for
event '%s'",
- event);
- r = -1;
- }
- if (r == 0)
+ if (run_event(run_state, dump_dir_name, event, &l_state) == 0)
{
printf("%s: ", event);
if (l_state.last_line)
@@ -560,14 +569,11 @@ static int run_events(const char *dump_dir_name, GList *events,
const char *even
event,
l_state.last_line ? ": " : "",
l_state.last_line ? l_state.last_line : ""
- );
- error_cnt++;
+ );
+ ++error_cnt;
}
free(l_state.last_line);
l_state.last_line = NULL;
-
- // Unexport overridden settings
- unexport_event_config(env_list);
}
free_run_event_state(run_state);
@@ -742,6 +748,51 @@ int collect(const char *dump_dir_name, int batch)
return errors;
}
+static int review_problem_data(const char *dump_dir_name, problem_data_t *problem_data)
+{
+ /* Open text editor and give a chance to review the backtrace etc */
+ create_fields_for_editor(problem_data);
+ int result = run_report_editor(problem_data);
+ if (result != 0)
+ return 1;
+
+ /* Save comment, backtrace */
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (dd)
+ {
+ //TODO: we should iterate through problem_data and modify all modifiable fields
+ const char *comment = problem_data_get_content_or_NULL(problem_data,
FILENAME_COMMENT);
+ const char *backtrace = problem_data_get_content_or_NULL(problem_data,
FILENAME_BACKTRACE);
+ if (comment)
+ dd_save_text(dd, FILENAME_COMMENT, comment);
+ if (backtrace)
+ dd_save_text(dd, FILENAME_BACKTRACE, backtrace);
+ dd_close(dd);
+ }
+
+ return 0;
+}
+
+static int is_backtrace_rating_usable(event_config_t *config, problem_data_t
*problem_data)
+{
+ char *usability_description = NULL;
+ char *usability_detail = NULL;
+ const bool usable_rating = check_problem_rating_usability(config, problem_data,
+
&usability_description,
+ &usability_detail);
+
+ if (!usable_rating)
+ {
+ printf("%s\n", usability_description);
+ printf("%s\n", usability_detail);
+ }
+
+ free(usability_description);
+ free(usability_detail);
+
+ return usable_rating;
+}
+
/* Report the crash */
int report(const char *dump_dir_name, int flags)
{
@@ -804,30 +855,12 @@ int report(const char *dump_dir_name, int flags)
problem_data_t *problem_data = create_problem_data_from_dump_dir(dd);
dd_close(dd);
- if (!(flags & (CLI_REPORT_BATCH)))
+ if (!(flags & (CLI_REPORT_BATCH))
+ && review_problem_data(dump_dir_name, problem_data))
{
- /* Open text editor and give a chance to review the backtrace etc */
- create_fields_for_editor(problem_data);
- int result = run_report_editor(problem_data);
- if (result != 0)
- {
- problem_data_free(problem_data);
- free(report_events_as_lines);
- return 1;
- }
- /* Save comment, backtrace */
- dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
- if (dd)
- {
-//TODO: we should iterate through problem_data and modify all modifiable fields
- const char *comment = problem_data_get_content_or_NULL(problem_data,
FILENAME_COMMENT);
- const char *backtrace = problem_data_get_content_or_NULL(problem_data,
FILENAME_BACKTRACE);
- if (comment)
- dd_save_text(dd, FILENAME_COMMENT, comment);
- if (backtrace)
- dd_save_text(dd, FILENAME_BACKTRACE, backtrace);
- dd_close(dd);
- }
+ problem_data_free(problem_data);
+ free(report_events_as_lines);
+ return 1;
}
/* Get possible reporters associated with this particular crash */
@@ -885,17 +918,8 @@ int report(const char *dump_dir_name, int flags)
if (!is_number_in_string(i, wanted_reporters))
continue;
- char *usability_description = NULL;
- char *usability_detail = NULL;
- const bool usable_rating = check_problem_rating_usability(config,
problem_data,
-
&usability_description,
-
&usability_detail);
-
- if (!usable_rating)
+ if (!is_backtrace_rating_usable(config, problem_data))
{
- printf("%s\n", usability_description);
- printf("%s\n", usability_detail);
-
errors++;
goto next_plugin;
}
@@ -914,8 +938,6 @@ int report(const char *dump_dir_name, int flags)
next_plugin:
plugins++;
- free(usability_description);
- free(usability_detail);
}
}
@@ -924,3 +946,98 @@ next_plugin:
list_free_with_free(report_events);
return errors;
}
+
+struct chain_logging_param
+{
+ struct logging_state l_state;
+ bool terminate;
+};
+
+static char *do_log_and_terminate_chain_on_request(char *log_line, void *param)
+{
+ struct chain_logging_param *arg = (struct chain_logging_param *)param;
+
+ arg->terminate = strcmp("THANKYOU", log_line) == 0;
+
+ return do_log_and_save_line(log_line, &(arg->l_state));
+}
+
+/*
+ * Runs event from chain. Perform the following steps for each event from chain.
+ * 1. Terminates a chain run if a backtrace is not usable.
+ * 2. Asks for missing settings.
+ * 3. Performs review of problem data if event requires review.
+ * 4. Runs events commands.
+ * 5. Terminates a chain run if any event from the chain requires termination
+ * (prints THANKYOU to stdout in the current implementation)
+ * 6. Terminates a chain run if any error occurs.
+ * 7. Continues immediately with processing of next event.
+ */
+int run_events_chain(const char *dump_dir_name, GList *chain)
+{
+ struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
+ if (!dd)
+ return -1;
+
+ problem_data_t *problem_data = create_problem_data_from_dump_dir(dd);
+
+ dd_close(dd);
+
+ struct chain_logging_param chain_state = {
+ .l_state = {
+ .last_line = NULL,
+ },
+ .terminate = false,
+ };
I would just memset it to 0. Code will be more readable this way (IMO).
+ struct run_event_state *run_state = new_run_event_state();
+ run_state->logging_callback = do_log_and_terminate_chain_on_request;
+ run_state->logging_param = &chain_state;
+
+ int returncode = 0;
+
+ for (GList *eitem = chain; !chain_state.terminate && eitem; eitem =
g_list_next(eitem))
+ {
+ const char *const event = (const char *)eitem->data;
+ event_config_t *config = get_event_config(event);
+
+ if (!is_backtrace_rating_usable(config, problem_data))
+ /* it is not a failure of event if the backtrace is unusable */
+ break;
+
+ if (config)
+ {
+ /* can't fail */
+ ask_for_missing_settings(event);
+
+ /* review problem data only if the event needs it */
+ if (!config->ec_skip_review
+ && review_problem_data(dump_dir_name, problem_data))
+ {
+ /* review failed, an error message was already logged */
+ returncode = -1;
+ break;
+ }
+ }
+
+ returncode = run_event(run_state, dump_dir_name, event,
&chain_state.l_state);
+ if (returncode != 0)
+ {
+ error_msg("'%s' event was not successful%s%s",
+ event,
+ chain_state.l_state.last_line ? ": " : "",
+ chain_state.l_state.last_line ? chain_state.l_state.last_line :
""
+ );
+ break;
+ }
+
+ free(chain_state.l_state.last_line);
+ chain_state.l_state.last_line = NULL;
+ }
+
+ free_run_event_state(run_state);
+ free(chain_state.l_state.last_line);
+ problem_data_free(problem_data);
+
+ return returncode;
+}
diff --git a/src/cli/cli-report.h b/src/cli/cli-report.h
index b34293d..8dd3773 100644
--- a/src/cli/cli-report.h
+++ b/src/cli/cli-report.h
@@ -32,6 +32,7 @@ enum {
};
int report(const char *dump_dir_name, int flags);
int collect(const char *dump_dir_name, int batch);
+int run_events_chain(const char *dump_dir_name, GList *chain);
#ifdef __cplusplus
}
diff --git a/src/cli/cli.c b/src/cli/cli.c
index 66ebbe2..5a221ad 100644
--- a/src/cli/cli.c
+++ b/src/cli/cli.c
@@ -23,10 +23,18 @@
#include "internal_libreport.h"
#include "cli-report.h"
-static char *do_log(char *log_line, void *param)
+static char *steal_directory_if_needed(char *dump_dir_name)
{
- log("%s", log_line);
- return log_line;
+ struct dump_dir *dd = open_directory_for_writing(dump_dir_name,
+ /* ask callback */ NULL);
+
+ if (dd)
+ {
+ dump_dir_name = xstrdup(dd->dd_dirname);
+ dd_close(dd);
+ }
+
+ return dump_dir_name;
}
int main(int argc, char** argv)
@@ -47,7 +55,7 @@ int main(int argc, char** argv)
textdomain(PACKAGE);
#endif
- const char *event_name = NULL;
+ GList *event_list = NULL;
const char *pfx = "";
/* Can't keep these strings/structs static: _() doesn't support that */
@@ -77,7 +85,7 @@ int main(int argc, char** argv)
struct options program_options[] = {
/* short_name long_name value parameter_name help */
OPT_OPTSTRING('L', NULL , &pfx, "PREFIX",
_("List possible events [which start with PREFIX]")),
- OPT_STRING( 'e', NULL , &event_name, "EVENT",
_("Run EVENT on DUMP_DIR")),
+ OPT_LIST( 'e', "event" , &event_list,
"EVENT", _("Run only these events")),
OPT_BOOL( 'a', "analyze", NULL,
_("Run analyze event(s) on DUMP_DIR")),
OPT_BOOL( 'c', "collect", NULL,
_("Run collect event(s) on DUMP_DIR")),
OPT_BOOL( 'r', "report" , NULL,
_("Analyze, collect and report problem data in DUMP_DIR")),
@@ -144,12 +152,8 @@ int main(int argc, char** argv)
}
case OPT_run_event: /* -e EVENT: run event */
{
- struct run_event_state *run_state = new_run_event_state();
- run_state->logging_callback = do_log;
- int r = run_event_on_dir_name(run_state, dump_dir_name, event_name);
- if (r == 0 && run_state->children_count == 0)
- error_msg_and_die("No actions are found for event
'%s'", event_name);
- free_run_event_state(run_state);
+ dump_dir_name = steal_directory_if_needed(dump_dir_name);
+ exitcode = run_events_chain(dump_dir_name, event_list);
break;
}
case OPT_analyze:
@@ -184,13 +188,7 @@ int main(int argc, char** argv)
}
case OPT_report:
{
- struct dump_dir *dd = open_directory_for_writing(dump_dir_name, NULL);
-
- if (dd)
- {
- dump_dir_name = xstrdup(dd->dd_dirname);
- dd_close(dd);
- }
+ dump_dir_name = steal_directory_if_needed(dump_dir_name);
exitcode = report(dump_dir_name,
(always ? CLI_REPORT_BATCH : 0));
Looks good to me.