more fixes to report C API (based on Denys's review)

3 files changed, 95 insertions, 73 deletions

diff --git a/src/gui-gtk/abrt-gtk.c b/src/gui-gtk/abrt-gtk.c
index 5fd29121..fe061829 100644
--- a/src/gui-gtk/abrt-gtk.c
+++ b/src/gui-gtk/abrt-gtk.c
@@ -195,7 +195,7 @@ static void on_button_send_cb(GtkWidget *button, gpointer data)
 
     problem_data_t *pd = new_problem_data();
 
-    if (strlen(text) > 0)
+    if (text[0])
     {
         add_to_problem_data(pd, "description", text);
     }
diff --git a/src/lib/problem_data.c b/src/lib/problem_data.c
index b4be1562..22c06ff2 100644
--- a/src/lib/problem_data.c
+++ b/src/lib/problem_data.c
@@ -63,6 +63,55 @@ void add_basics_to_problem_data(problem_data_t *pd)
     if (analyzer == NULL)
         add_to_problem_data(pd, "analyzer", "libreport");
 
+    /* If application didn't provide dupe hash, we generate it
+     * from all components, so we at least eliminate the exact same
+     * reports
+     */
+    if (get_problem_item_content_or_NULL(pd, FILENAME_DUPHASH) == NULL)
+    {
+        /* start hash */
+        static sha1_ctx_t sha1ctx;
+        unsigned char hash_bytes[SHA1_RESULT_LEN];
+        char hash_str[SHA1_RESULT_LEN*2 + 1];
+        sha1_begin(&sha1ctx);
+        /*
+         * To avoid spurious hash differences, sort keys so that elements are
+         * always processed in the same order:
+         */
+        GList *list = g_hash_table_get_keys(pd);
+        list = g_list_sort(list, (GCompareFunc)strcmp);
+        GList *l = list;
+        while (l)
+        {
+            const char *key = l->data;
+            l = l->next;
+            struct problem_item *item = g_hash_table_lookup(pd, key);
+            /* do not hash items which are binary (item->flags & CD_FLAG_BIN).
+             * Their ->content is full file name, with path. Path is always
+             * different and will make hash differ even if files are the same.
+             */
+            if (item->flags & CD_FLAG_BIN)
+                continue;
+            sha1_hash(&sha1ctx, item->content, strlen(item->content));
+        }
+        /* end hash */
+        sha1_end(&sha1ctx, hash_bytes);
+
+        unsigned len = SHA1_RESULT_LEN;
+        unsigned char *s = hash_bytes;
+        char *d = hash_str;
+        while (len)
+        {
+            *d++ = "0123456789abcdef"[*s >> 4];
+            *d++ = "0123456789abcdef"[*s & 0xf];
+            s++;
+            len--;
+        }
+        *d = '\0';
+
+        add_to_problem_data(pd, FILENAME_DUPHASH, hash_str);
+    }
+
     pid_t pid = getpid();
     if (pid > 0)
     {
@@ -78,11 +127,13 @@ void add_basics_to_problem_data(problem_data_t *pd)
         free(exe);
 
 //#ifdef WITH_RPM
-        /* FIXME: component should be taken from rpm using
-         * rpm -qf executable
+        /* FIXME: component should be taken from rpm using librpm
+         * which means we need to link against it :(
+         * or run rpm -qf executable ??
         */
         /* Fedora/RHEL rpm specific piece of code */
-        const char *component = get_problem_item_content_or_NULL(pd, FILENAME_ANALYZER);
+        const char *component = get_problem_item_content_or_NULL(pd, FILENAME_COMPONENT);
+        //FIXME: this REALLY needs to go away, or every report will be assigned to abrt
         if(component == NULL) // application didn't specify component
             add_to_problem_data(pd, FILENAME_COMPONENT, "abrt");
 //#endif
diff --git a/src/lib/report.c b/src/lib/report.c
index 08be48a8..d6b45fcf 100644
--- a/src/lib/report.c
+++ b/src/lib/report.c
@@ -20,38 +20,9 @@
 #include "abrtlib.h"
 #include "report.h"
 
-static void create_hash(char hash_str[SHA1_RESULT_LEN*2 + 1], const char *pInput)
-{
-    unsigned char hash_bytes[SHA1_RESULT_LEN];
-    sha1_ctx_t sha1ctx;
-    sha1_begin(&sha1ctx);
-    sha1_hash(&sha1ctx, pInput, strlen(pInput));
-    sha1_end(&sha1ctx, hash_bytes);
-
-    unsigned len = SHA1_RESULT_LEN;
-    unsigned char *s = hash_bytes;
-    char *d = hash_str;
-    while (len)
-    {
-        *d++ = "0123456789abcdef"[*s >> 4];
-        *d++ = "0123456789abcdef"[*s & 0xf];
-        s++;
-        len--;
-    }
-    *d = '\0';
-    //log("hash:%s str:'%s'", hash_str, pInput);
-}
-
-static void generate_hash_for_all(gpointer key, gpointer value, gpointer user_data)
-{
-    problem_item *pi = (problem_item *)value;
-    char *hash_str = (char *)user_data;
-    create_hash(hash_str, pi->content);
-}
-
 static int run_reporter_ui(char **args, int flags)
 {
-    char path[PATH_MAX+1];
+    const char *path;
     /*
     if is isatty
       -> run cli reporter
@@ -62,19 +33,19 @@ static int run_reporter_ui(char **args, int flags)
     if (pid == 0)
     {
         /* Some callers set SIGCHLD to SIG_IGN.
-         * However, reporting spawns chils processes.
-         * Suppressing chil death notification terribly confuses some of them.
+         * However, reporting spawns child processes.
+         * Suppressing child death notification terribly confuses some of them.
          * Just in case, undo it.
          * Note that we do it in the child, so the parent is never affected.
          */
-        signal(SIGCHLD, SIG_DFL); // applet still set it to SIG_IGN
-        strncpy(path, BIN_DIR"/bug-reporting-wizard", PATH_MAX);
-        path[PATH_MAX] = 0;
+        signal(SIGCHLD, SIG_DFL);
+        path = BIN_DIR"/bug-reporting-wizard";
         VERB1 log("Executing: %s", path);
         execv(path, args);
-        /* Did not find abrt-gui in installation directory. Oh well */
-        /* Trying to find it in PATH */
-        strncpy(path, "bug-reporting-wizard", PATH_MAX);
+        /* Did not find the desired executable in the installation directory.
+         * Trying to find it in PATH
+         */
+        path = "bug-reporting-wizard";
         execvp(path, args);
         perror_msg_and_die("Can't execute %s", "bug-reporting-wizard");
     }
@@ -82,11 +53,11 @@ static int run_reporter_ui(char **args, int flags)
     {
         if (flags & WAIT)
         {
-            int status = 0;
-            pid_t p = waitpid(pid, &status, WUNTRACED);
-            if(p == -1)
+            int status;
+            pid_t p = waitpid(pid, &status, 0);
+            if(p <= 0)
             {
-                error_msg("can't waitpid");
+                perror_msg("can't waitpid");
                 return EXIT_FAILURE;
             }
             if (WIFEXITED(status))
@@ -94,18 +65,10 @@ static int run_reporter_ui(char **args, int flags)
                 VERB2 log("reporting finished with exitcode: status=%d\n", WEXITSTATUS(status));
                 return WEXITSTATUS(status);
             }
-            else if (WIFSIGNALED(status))
+            else // (WIFSIGNALED(status))
             {
                 VERB2 log("reporting killed by signal %d\n", WTERMSIG(status));
-            }
-            else if (WIFSTOPPED(status))
-            {
-                /* should parent continue when the reporting is stopped??*/
-                VERB2 log("reporting stopped by signal %d\n", WSTOPSIG(status));
-            }
-            else if (WIFCONTINUED(status))
-            {
-                VERB2 log("continued\n");
+                return WTERMSIG(status) + 128;
             }
         }
     }
@@ -134,21 +97,23 @@ int analyze_and_report(problem_data_t *pd, int flags)
     struct dump_dir *dd = create_dump_dir_from_problem_data(pd, "/tmp"/* /var/tmp ?? */);
     if (!dd)
         return -1;
-    char *dir_name = strdup(dd->dd_dirname);
+    char *dir_name = xstrdup(dd->dd_dirname);
     dd_close(dd);
     VERB2 log("Temp problem dir: '%s'\n", dir_name);
     result = analyze_and_report_dir(dir_name, flags);
 
-    /* if we wait for reporter to finish, we can try to clean the tmp dir */
+    /* if we wait for reporter to finish, we can clean the tmp dir
+     * and we should reload the problem data, so caller doesn't see the stalled
+     * data
+    */
     if (flags & WAIT)
     {
+        g_hash_table_remove_all(pd);
         dd = dd_opendir(dir_name, 0);
         if (dd)
         {
-            if (dd_delete(dd) != 0)
-            {
-                error_msg("Can't remove tmp dir: %s", dd->dd_dirname);
-            }
+            load_problem_data_from_dump_dir(pd, dd);
+            dd_delete(dd);
         }
     }
     free(dir_name);
@@ -174,19 +139,13 @@ int report_dir(const char* dirname)
     args[4] = NULL;
 
     int flags = WAIT;
-    run_reporter_ui(args, flags);
-    return 0;
+    int status;
+    status = run_reporter_ui(args, flags);
+    return status;
 }
 
 int report(problem_data_t *pd)
 {
-    /* create hash from all components, so we at least eliminate the exact same
-     * reports
-    */
-    char hash_str[SHA1_RESULT_LEN*2 + 1];
-    g_hash_table_foreach(pd, &generate_hash_for_all, hash_str);
-    add_to_problem_data(pd, FILENAME_DUPHASH, hash_str);
-
     /* adds:
      *  analyzer:libreport
      *  executable:readlink(/proc/<pid>/exe)
@@ -200,8 +159,20 @@ int report(problem_data_t *pd)
     char *dir_name = xstrdup(dd->dd_dirname);
     dd_close(dd);
     VERB2 log("Temp problem dir: '%s'\n", dir_name);
-    report_dir(dir_name);
+    int result = report_dir(dir_name);
+
+    /* here we always wait for reporter to finish, we can clean the tmp dir
+     * and we should reload the problem data, so caller doesn't see the stalled
+     * data
+    */
+    g_hash_table_remove_all(pd); //what if something fails? is it ok to return empty pd rather then stalled pd?
+    dd = dd_opendir(dir_name, 0);
+    if (dd)
+    {
+        load_problem_data_from_dump_dir(pd, dd);
+        dd_delete(dd);
+    }
     free(dir_name);
 
-    return 0;
+    return result;
 }