On Wednesday 25 of July 2012 14:50:45 Denys Vlasenko wrote:
> On 07/20/2012 02:22 PM, Jakub Filak wrote:
> > * unification of implementaion of report-gtk and report-cli
> >
> > Signed-off-by: Jakub Filak <[email protected]>
> > ---
> >
> > src/cli/cli-report.c | 29 ++++++++---------
> > src/gui-wizard-gtk/wizard.c | 55 +++++---------------------------
> > src/include/event_config.h | 22 +++++++++++++
> > src/lib/event_config.c | 73
> > +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 117
> > insertions(+), 62 deletions(-)
> >
> > diff --git a/src/cli/cli-report.c b/src/cli/cli-report.c
> > index 117c6fb..988bfc7 100644
> > --- a/src/cli/cli-report.c
> > +++ b/src/cli/cli-report.c
> > @@ -857,12 +857,7 @@ int report(const char *dump_dir_name, int flags)
> >
> > }
> > else
> > {
> >
> > - const char *rating_str =
> > problem_data_get_content_or_NULL(problem_data, FILENAME_RATING);
> > -//COMPAT, remove after 2.1 release
> > - if (!rating_str)
> > - rating_str = problem_data_get_content_or_NULL(problem_data,
> > "rating"); -
> > - unsigned i, rating = rating_str ? xatou(rating_str) : 4;
> > + unsigned i;
> >
> > GList *li;
> > char wanted_reporters[255];
> >
> > @@ -890,18 +885,21 @@ int report(const char *dump_dir_name, int flags)
> >
> > if (!is_number_in_string(i, wanted_reporters))
> >
> > continue;
> >
> > - if (config && (rating < config->ec_minimal_rating))
> > - {
> > - puts(_("Reporting disabled because the backtrace is
> > unusable")); + char *usability_description = NULL;
> > + char *usability_detail = NULL;
> > + const bool usable_rating =
> > check_problem_rating_usability(config, problem_data, +
> > &usability_description,
> > +
> > &usability_detail);
> >
> > - const char *package =
> > problem_data_get_content_or_NULL(problem_data, FILENAME_PACKAGE); -
> > if (package && package[0])
> > - printf(_("Please try to install debuginfo manually
> > using the command: \"debuginfo-install %s\" and try again\n"), package);
> > + if (!usable_rating)
> > + {
> > + printf("%s\n", usability_description);
> > + printf("%s\n", usability_detail);
> >
> > - plugins++;
> >
> > errors++;
> >
> > - continue;
> > + goto next_plugin;
> >
> > }
> >
> > +
> >
> > ask_for_missing_settings(reporter_name);
> >
> > /*
> >
> > @@ -914,7 +912,10 @@ int report(const char *dump_dir_name, int flags)
> >
> > errors += run_events(dump_dir_name, cur_event, "Reporting");
> > g_list_free(cur_event);
> >
> > +next_plugin:
> > plugins++;
> >
> > + free(usability_description);
> > + free(usability_detail);
> >
> > }
> >
> > }
> >
> > diff --git a/src/gui-wizard-gtk/wizard.c b/src/gui-wizard-gtk/wizard.c
> > index c93ff42..a32aafa 100644
> > --- a/src/gui-wizard-gtk/wizard.c
> > +++ b/src/gui-wizard-gtk/wizard.c
> > @@ -1644,61 +1644,20 @@ static bool check_minimal_bt_rating(const char
> > *event_name)>
> > if (!event_name)
> >
> > error_msg_and_die(_("Cannot check backtrace rating because of
> > invalid event name"));>
> > - else if(strncmp("report", event_name, sizeof("report")-1) != 0)
> > + else if (prefixcmp(event_name, "report") != 0)
> >
> > {
> >
> > - VERB1 log("No checks for bactrace rating because event '%s'
> > doesn't report.", event_name); + VERB2 log("No checks for bactrace
> > rating because event '%s' doesn't report.", event_name);>
> > return acceptable_rating;
> >
> > }
> > else
> >
> > - {
> >
> > event_cfg = get_event_config(event_name);
> >
> > - if (!event_cfg)
> > - {
> > - VERB1 log("Cannot check backtrace rating because of not
> > defined configuration for event '%s'", event_name); - return
> > acceptable_rating;
> > - }
> > - }
> > -
> > - /*
> > - * FIXME: this should be bind to a reporter not to a compoment
> > - * but so far only oopses don't have rating, so for now we
> > - * skip the "kernel" manually
> > - */
> > - const char *analyzer = problem_data_get_content_or_NULL(g_cd,
> > FILENAME_ANALYZER); -//FIXME: say "no" to special casing!
> > - if (event_cfg && analyzer && strcmp(analyzer, "Kerneloops") != 0)
> > + char *description = NULL;
> > + acceptable_rating = check_problem_rating_usability(event_cfg, g_cd,
> > &description, NULL); + if (description)
> >
> > {
> >
> > - const char *rating_str = problem_data_get_content_or_NULL(g_cd,
> > FILENAME_RATING); -//COMPAT, remove after 2.1 release
> > - if (!rating_str)
> > - rating_str = problem_data_get_content_or_NULL(g_cd,
> > "rating");
> > -
> > - if (rating_str)
> > - {
> > - char *endptr;
> > - errno = 0;
> > - long rating = strtol(rating_str, &endptr, 10);
> > - if (errno != 0 || endptr == rating_str || *endptr != '\0')
> > - {
> > - add_warning(_("Reporting disabled because the rating does
> > not contain a number.")); - acceptable_rating = false;
> > - }
> > - else
> > - {
> > - VERB1 log("Checking current rating %ld to required rating
> > %ld.", rating, event_cfg->ec_minimal_rating); - if (rating
> > == event_cfg->ec_minimal_rating) /* bt is usable, but not complete, so
> > show a warning */ - {
> > - add_warning(_("The backtrace is incomplete, please
> > make sure you provide the steps to reproduce.")); - }
> > - else if (rating < event_cfg->ec_minimal_rating)
> > - {
> > - add_warning(_("Reporting disabled because the
> > backtrace is unusable.")); - //FIXME: see
> > CreporterAssistant: 394 for ideas - acceptable_rating
> > = false;
> > - }
> > - }
> > - }
> > + add_warning(description);
> > + free(description);
> >
> > }
> >
> > return acceptable_rating;
> >
> > diff --git a/src/include/event_config.h b/src/include/event_config.h
> > index 6644b42..15ebf52 100644
> > --- a/src/include/event_config.h
> > +++ b/src/include/event_config.h
> > @@ -21,6 +21,7 @@
> >
> > #include <stdbool.h>
> > #include <glib.h>
> >
> > +#include "problem_data.h"
> >
> > #ifdef __cplusplus
> > extern "C" {
> >
> > @@ -103,6 +104,27 @@ void unexport_event_config(GList *env_list);
> >
> > GHashTable *validate_event(const char *event_name);
> >
> > +/*
> > + * Checks usability of problem's backtrace rating against required rating
> > level + * from event configuration.
> > + *
> > + * @param cfg an event configuration
> > + * @param pd a checked problem data
> > + * @param description an output parameter for a description of rating
> > + * usability. If the variable holds NULL after function call no
> > description is + * available. The description can be provided even if
> > backtrace rating is + * acceptable. Can be NULL.
> > + * @param detail an output parameter for a more details about rating
> > usability. + * If the variable holds NULL after function call no
> > description is available. + * The detail can be provided even if
> > backtrace rating is acceptable. Can be + * NULL.
> > + * @returns true if rating is usable or above usable; otherwise false
> > + */
> > +bool check_problem_rating_usability(const event_config_t *cfg,
> > + problem_data_t *pd,
> > + char **description,
> > + char **detail);
> > +
> >
> > #ifdef __cplusplus
> > }
> > #endif
> >
> > diff --git a/src/lib/event_config.c b/src/lib/event_config.c
> > index 85c0e80..9bf0d5f 100644
> > --- a/src/lib/event_config.c
> > +++ b/src/lib/event_config.c
> > @@ -367,3 +367,76 @@ GHashTable *validate_event(const char *event_name)
> >
> > return NULL;
> >
> > }
> >
> > +
> > +/*
> > + * This function checks if a value of problem's backtrace rating is
> > acceptable + * for event according to event's required rating value.
> > + *
> > + * The solved problem seems to be pretty straightforward but there are
> > some + * pitfalls.
> > + *
> > + * 1. Is rating acceptable if event doesn't have configuration?
> > + * 2. Is rating acceptable if problem's data doesn't contains rating
> > value? + * 3. Is rating acceptable if rating value is not a number?
> > + * 4. What message show to user if there is a concern about usability?
> > + */
> > +bool check_problem_rating_usability(const event_config_t *cfg,
> > + problem_data_t *pd,
> > + char **description,
> > + char **detail)
> > +{
> > + char *tmp_desc = NULL;
> > + char *tmp_detail = NULL;
> > +
> > + bool result = false;
> > +
> > + const char *rating_str = problem_data_get_content_or_NULL(pd,
> > FILENAME_RATING); + if (!cfg || !rating_str)
> > + goto finish;
>
> You can do "if (!cfg) goto finish;" even before getting FILENAME_RATING.
You are right. My intention was to have only one "if () goto". I wasn't
decided which construction is better. I'll use the proposed.
>
> > + const long minimal_rating = cfg->ec_minimal_rating;
> > + char *endptr;
> > + errno = 0;
> > + const long rating = strtol(rating_str, &endptr, 10);
> > + if (errno != 0 || endptr == rating_str || *endptr != '\0')
> > + {
> > + tmp_desc = xasprintf("%s '%s'.", _("Reporting disabled because
> > the rating does not contain a number."), rating_str); + tmp_detail
> > = xstrdup(_("Please report this problem to ABRT."));
> Maybe "Please report this problem to ABRT project developers" ?
OK, it is better.
>
> > +
> > + result = false;
> > + }
> > + else if (rating == minimal_rating) /* bt is usable, but not complete,
> > so show a warning */
> What if minimal_rating == highest possible rating?
> Will we _always_ say "The backtrace is incomplete" in this case?
Yes, we will. The minimal rating is loaded from the configuration. Should we
take care about this weird configuration? AFAIK we don't care in the current
implementation.
>
> > + {
> > + tmp_desc = xstrdup(_("The backtrace is incomplete, please make
> > sure you provide the steps to reproduce.")); + tmp_detail =
> > xstrdup(_("The backtrace probably can't help to a developer in searching
> > for bug.")); +
> > + result = true;
> > + }
> > + else if (rating < minimal_rating)
> > + {
> > + tmp_desc = xstrdup(_("Reporting disabled because the backtrace is
> > unusable.")); +
> > + const char *package = problem_data_get_content_or_NULL(pd,
> > FILENAME_PACKAGE); + if (package && package[0])
> > + tmp_detail = xasprintf(_("Please try to install debuginfo
> > manually using the command: \"debuginfo-install %s\" and try again."),
> > package); + else
> > + tmp_detail = xstrdup(_("A proppert debuginfo is probably
> > missing or the coredump is corrupted."));
> s/proppert/proper/
>
> > +
> > + result = false;
> > + }
> > + else /* rating > minimal_rating */
> > + result = true;
> > +
> > +finish:
> > + if (description)
> > + *description = tmp_desc;
> > + else
> > + free(tmp_desc);
> > +
> > + if (detail)
> > + *detail = tmp_detail;
> > + else
> > + free(tmp_detail);
> > +
> > + return result;
> > +}
Regards,
Jakub
