[PATCH 1/2] trac#622 : move bt rating check to a library function

[Jakub Filak]

* unification of implementaion of report-gtk and report-cli

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 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;
+
+    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."));
+
+        result = false;
+    }
+    else if (rating == minimal_rating) /* bt is usable, but not complete, so show a
warning */
+    {
+        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."));
+
+        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;
+}
-- 
1.7.10.4

[PATCH 2/2] trac#622 : add open_directory_for_writing()

[Jakub Filak]

* unify implementaions of stealing of dump directory from report-gtk and
  report-cli

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 src/cli/cli.c                    |   28 ++-------
 src/gui-wizard-gtk/wizard.c      |  121 +++++++++++++++-----------------------
 src/include/internal_libreport.h |   11 ++++
 src/lib/steal_directory.c        |   42 +++++++++++++
 4 files changed, 106 insertions(+), 96 deletions(-)

diff --git a/src/cli/cli.c b/src/cli/cli.c
index 0f38734..66ebbe2 100644
--- a/src/cli/cli.c
+++ b/src/cli/cli.c
@@ -47,7 +47,6 @@ int main(int argc, char** argv)
     textdomain(PACKAGE);
 #endif
 
-    GList *D_list = NULL;
     const char *event_name = NULL;
     const char *pfx = "";
 
@@ -124,12 +123,6 @@ int main(int argc, char** argv)
     char *dump_dir_name = argv[0];
     bool always = (opts & OPT_y);
 
-    if (!D_list)
-    {
-        D_list = g_list_append(D_list, concat_path_file(g_get_user_cache_dir(),
"abrt/spool"));
-        D_list = g_list_append(D_list, (void*)DEBUG_DUMPS_DIR);
-    }
-
     /* Get settings */
     load_event_config_data();
 
@@ -191,26 +184,17 @@ int main(int argc, char** argv)
         }
         case OPT_report:
         {
-            struct dump_dir *dd = dd_opendir(dump_dir_name, DD_OPEN_READONLY);
-            if (!dd)
-                return 1;
-            int readonly = !dd->locked;
-            dd_close(dd);
-            if (readonly)
+            struct dump_dir *dd = open_directory_for_writing(dump_dir_name, NULL);
+
+            if (dd)
             {
-                log("'%s' is not writable", dump_dir_name);
-                /* D_list can't be NULL here */
-                struct dump_dir *dd_copy = steal_directory((char *)D_list->data,
dump_dir_name);
-                if (dd_copy)
-                {
-                    delete_dump_dir_possibly_using_abrtd(dump_dir_name);
-                    dump_dir_name = xstrdup(dd_copy->dd_dirname);
-                    dd_close(dd_copy);
-                }
+                dump_dir_name = xstrdup(dd->dd_dirname);
+                dd_close(dd);
             }
 
             exitcode = report(dump_dir_name,
                     (always ? CLI_REPORT_BATCH : 0));
+
             if (exitcode == -1)
                 error_msg_and_die("Crash '%s' not found",
dump_dir_name);
 
diff --git a/src/gui-wizard-gtk/wizard.c b/src/gui-wizard-gtk/wizard.c
index a32aafa..cccf17f 100644
--- a/src/gui-wizard-gtk/wizard.c
+++ b/src/gui-wizard-gtk/wizard.c
@@ -311,72 +311,53 @@ static void on_toggle_ask_steal_cb(GtkToggleButton *tb, gpointer
user_data)
     set_user_setting("ask_steal_dir", gtk_toggle_button_get_active(tb) ?
"no" : "yes");
 }
 
-struct dump_dir *steal_if_needed(struct dump_dir *dd)
+static bool ask_continue_before_steal(const char *base_dir, const char *dump_dir)
 {
-    if (!dd)
-        xfunc_die(); /* error msg was already logged */
-
-    if (dd->locked)
-        return dd;
-
-    dd_close(dd);
-
-    char *spooldir;
-    spooldir = concat_path_file(g_get_user_cache_dir(), "abrt/spool");
-
     const char *ask_steal_dir = get_user_setting("ask_steal_dir");
 
-    if (!ask_steal_dir || strcmp(ask_steal_dir, "no"))
-    {
-        GtkWidget *dialog = gtk_message_dialog_new(GTK_WINDOW(g_wnd_assistant),
-                GTK_DIALOG_DESTROY_WITH_PARENT,
-                GTK_MESSAGE_QUESTION,
-                GTK_BUTTONS_OK_CANCEL,
-                _("Need writable directory, but '%s' is not writable."
-                " Move it to '%s' and operate on the moved data?"),
-                g_dump_dir_name, spooldir
-                );
-        gint response = GTK_RESPONSE_CANCEL;
-        g_signal_connect(G_OBJECT(dialog), "response",
G_CALLBACK(save_dialog_response), &response);
+    if (ask_steal_dir && !strcmp(ask_steal_dir, "no"))
+        return true;
 
-        GtkWidget *ask_steal_cb = gtk_check_button_new_with_label(_("Don't ask
me again"));
-        gtk_box_pack_start(GTK_BOX(gtk_dialog_get_content_area(GTK_DIALOG(dialog))),
-                ask_steal_cb, TRUE, TRUE, 0);
-        g_signal_connect(ask_steal_cb, "toggled",
G_CALLBACK(on_toggle_ask_steal_cb), NULL);
+    GtkWidget *dialog = gtk_message_dialog_new(GTK_WINDOW(g_wnd_assistant),
+            GTK_DIALOG_DESTROY_WITH_PARENT,
+            GTK_MESSAGE_QUESTION,
+            GTK_BUTTONS_OK_CANCEL,
+            _("Need writable directory, but '%s' is not writable."
+            " Move it to '%s' and operate on the moved data?"),
+            dump_dir, base_dir
+            );
 
-        /* check it by default if it's shown for the first time */
-        if (!ask_steal_dir) {
-            gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(ask_steal_cb), TRUE);
-	}
+    gint response = GTK_RESPONSE_CANCEL;
+    g_signal_connect(G_OBJECT(dialog), "response",
G_CALLBACK(save_dialog_response), &response);
 
-        gtk_widget_show(ask_steal_cb);
-        gtk_dialog_run(GTK_DIALOG(dialog));
-        gtk_widget_destroy(dialog);
+    GtkWidget *ask_steal_cb = gtk_check_button_new_with_label(_("Don't ask me
again"));
+    gtk_box_pack_start(GTK_BOX(gtk_dialog_get_content_area(GTK_DIALOG(dialog))),
+            ask_steal_cb, TRUE, TRUE, 0);
+    g_signal_connect(ask_steal_cb, "toggled",
G_CALLBACK(on_toggle_ask_steal_cb), NULL);
 
-        if (response != GTK_RESPONSE_OK)
-            return NULL;
-    }
+    /* check it by default if it's shown for the first time */
+    if (!ask_steal_dir)
+        gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(ask_steal_cb), TRUE);
 
-    dd = steal_directory(spooldir, g_dump_dir_name);
-    g_free(spooldir);
-    if (!dd)
-        return NULL; /* Stealing failed. Error msg was already logged */
+    gtk_widget_show(ask_steal_cb);
+    gtk_dialog_run(GTK_DIALOG(dialog));
+    gtk_widget_destroy(dialog);
 
-    /* Delete old dir and switch to new one.
-     * Don't want to keep new dd open across deletion,
-     * therefore it's a bit more complicated.
-     */
-    char *old_name = g_dump_dir_name;
-    g_dump_dir_name = xstrdup(dd->dd_dirname);
-    dd_close(dd);
+    return response == GTK_RESPONSE_OK;
+}
 
-    update_window_title();
-    delete_dump_dir_possibly_using_abrtd(old_name); //TODO: if (deletion_failed)
error_msg("BAD")?
-    free(old_name);
+struct dump_dir *wizard_open_directory_for_writing(const char *dump_dir_name)
+{
+    struct dump_dir *dd = open_directory_for_writing(dump_dir_name,
+                                                     ask_continue_before_steal);
 
-    dd = dd_opendir(g_dump_dir_name, 0);
-    if (!dd)
-        xfunc_die(); /* error msg was already logged */
+    if (dd && strcmp(g_dump_dir_name, dd->dd_dirname) != 0)
+    {
+        char *old_name = g_dump_dir_name;
+        g_dump_dir_name = xstrdup(dd->dd_dirname);
+        update_window_title();
+        free(old_name);
+    }
 
     return dd;
 }
@@ -439,12 +420,10 @@ static void save_text_if_changed(const char *name, const char
*new_value)
         old_value = "";
     if (strcmp(new_value, old_value) != 0)
     {
-        struct dump_dir *dd = dd_opendir(g_dump_dir_name, DD_OPEN_READONLY);
-        dd = steal_if_needed(dd);
-        if (dd && dd->locked)
-        {
+        struct dump_dir *dd = wizard_open_directory_for_writing(g_dump_dir_name);
+        if (dd)
             dd_save_text(dd, name, new_value);
-        }
+
 //FIXME: else: what to do with still-unsaved data in the widget??
         dd_close(dd);
         problem_data_reload_from_dump_dir();
@@ -1544,11 +1523,9 @@ static void start_event_run(const char *event_name,
         return;
     }
 
-    struct dump_dir *dd = dd_opendir(g_dump_dir_name, DD_OPEN_READONLY);
-    dd = steal_if_needed(dd);
-    int locked = (dd && dd->locked);
+    struct dump_dir *dd = wizard_open_directory_for_writing(g_dump_dir_name);
     dd_close(dd);
-    if (!locked)
+    if (!dd)
     {
         free_run_event_state(state);
         return; /* user refused to steal, or write error, etc... */
@@ -2142,9 +2119,8 @@ static void save_edited_one_liner(GtkCellRendererText *renderer,
     struct problem_item *item = problem_data_get_item_or_NULL(g_cd, item_name);
     if (item && (item->flags & CD_FLAG_ISEDITABLE))
     {
-        struct dump_dir *dd = dd_opendir(g_dump_dir_name, DD_OPEN_READONLY);
-        dd = steal_if_needed(dd);
-        if (dd && dd->locked)
+        struct dump_dir *dd = wizard_open_directory_for_writing(g_dump_dir_name);
+        if (dd)
         {
             dd_save_text(dd, item_name, new_text);
             free(item->content);
@@ -2193,11 +2169,8 @@ static void on_btn_add_file(GtkButton *button)
         struct problem_item *item = problem_data_get_item_or_NULL(g_cd, basename);
         if (!item || (item->flags & CD_FLAG_ISEDITABLE))
         {
-            struct dump_dir *dd = dd_opendir(g_dump_dir_name, DD_OPEN_READONLY);
-            dd = steal_if_needed(dd);
-            bool writable = (dd && dd->locked);
-            dd_close(dd);
-            if (writable)
+            struct dump_dir *dd = wizard_open_directory_for_writing(g_dump_dir_name);
+            if (dd)
             {
                 char *new_name = concat_path_file(g_dump_dir_name, basename);
                 if (strcmp(filename, new_name) == 0)
@@ -2222,6 +2195,7 @@ static void on_btn_add_file(GtkButton *button)
                 }
                 free(new_name);
             }
+            dd_close(dd);
         }
         else
             message = xasprintf(_("Item '%s' already exists and is not
modifiable"), basename);
@@ -2264,8 +2238,7 @@ static void delete_item(GtkTreeView *treeview)
                 {
 //                  GtkTreePath *old_path = gtk_tree_model_get_path(store, &iter);
 
-                    struct dump_dir *dd = dd_opendir(g_dump_dir_name, DD_OPEN_READONLY);
-                    dd = steal_if_needed(dd);
+                    struct dump_dir *dd =
wizard_open_directory_for_writing(g_dump_dir_name);
                     if (dd)
                     {
                         char *filename = concat_path_file(g_dump_dir_name, item_name);
diff --git a/src/include/internal_libreport.h b/src/include/internal_libreport.h
index 9d27a0d..e0cd7e6 100644
--- a/src/include/internal_libreport.h
+++ b/src/include/internal_libreport.h
@@ -634,6 +634,17 @@ int delete_dump_dir_possibly_using_abrtd(const char *dump_dir_name);
 #define steal_directory libreport_steal_directory
 struct dump_dir *steal_directory(const char *base_dir, const char *dump_dir_name);
 
+/* Tries to open dump_dir_name with writing access. If function needs to steal
+ * directory calls ask_continue(new base dir, dump dir) callback to ask user
+ * for permission. If ask_continue param is NULL the function thinks that an
+ * answer is positive and steals directory.
+ * Returns NULL if opening failed or if stealing was dismissed. In this case,
+ * logs a message before returning. */
+#define open_directory_for_writing libreport_open_directory_for_writing
+struct dump_dir *open_directory_for_writing(
+                         const char *dump_dir_name,
+                         bool (*ask_continue)(const char *, const char *));
+
 // Files bigger than this are never considered to be text.
 //
 // Started at 64k limit. But _some_ limit is necessary:
diff --git a/src/lib/steal_directory.c b/src/lib/steal_directory.c
index 1281872..ccfdd30 100644
--- a/src/lib/steal_directory.c
+++ b/src/lib/steal_directory.c
@@ -75,3 +75,45 @@ struct dump_dir *steal_directory(const char *base_dir, const char
*dump_dir_name
 
     return dd_dst;
 }
+
+struct dump_dir *open_directory_for_writing(
+                            const char *dump_dir_name,
+                            bool (*ask)(const char *, const char *))
+{
+    struct dump_dir *dd = dd_opendir(dump_dir_name, DD_OPEN_READONLY);
+
+    if (!dd)
+        xfunc_die(); /* error msg was already logged */
+
+    if (dd->locked)
+        return dd;
+
+    log("'%s' is not writable", dump_dir_name);
+    dd_close(dd);
+
+    char *spooldir = concat_path_file(g_get_user_cache_dir(), "abrt/spool");
+
+    if (ask && !ask(spooldir, dump_dir_name))
+        return NULL;
+
+    dd = steal_directory(spooldir, dump_dir_name);
+    free(spooldir);
+
+    if (!dd)
+        return NULL;
+
+    /* Delete old dir and switch to new one.
+     * Don't want to keep new dd open across deletion,
+     * therefore it's a bit more complicated.
+     */
+    delete_dump_dir_possibly_using_abrtd(dump_dir_name);
+    char *new_name = xstrdup(dd->dd_dirname);
+    dd_close(dd);
+    dd = dd_opendir(new_name, 0);
+    free(new_name);
+
+    if (!dd)
+        xfunc_die(); /* error msg was already logged */
+
+    return dd;
+}
-- 
1.7.10.4

[PATCH 2/2] trac#622 : add open_directory_for_writing()

[Denys Vlasenko]

On 07/20/2012 02:22 PM, Jakub Filak wrote:
>  +    if (ask_steal_dir && !strcmp(ask_steal_dir,
> "no"))
>  +        return true; 
Maybe s/!strcmp(ask_steal_dir, "no")/string_to_bool(ask_steal_dir) == false/ ?

[PATCH 2/2] trac#622 : add open_directory_for_writing()

[Jakub Filak]

On Wednesday 25 of July 2012 14:55:53 Denys Vlasenko wrote:
>  On 07/20/2012 02:22 PM, Jakub Filak wrote:
>  > +    if (ask_steal_dir && !strcmp(ask_steal_dir, "no"))
>  > +        return true;
>  
>  Maybe s/!strcmp(ask_steal_dir, "no")/string_to_bool(ask_steal_dir) == false/
>  ? 
It looks much better.

[Denys Vlasenko]

On 07/20/2012 02:22 PM, Jakub Filak wrote:
>  * unification of implementaion of report-gtk and report-cli
>  
>  Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
>  ---
>   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.

>  +    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"   ?

>  +
>  +        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?

>  +    {
>  +        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;
>  +}

[Jakub Filak]

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 <jfilak(a)redhat.com&gt;
>  > ---
>  > 
>  >  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