From d9f0726cbc8daec464405355ee943db5988986a7 Mon Sep 17 00:00:00 2001 From: Jiri Moskovcak Date: Fri, 20 May 2011 14:04:16 +0200 Subject: more fixes to report C API (based on Denys's review) --- src/gui-gtk/abrt-gtk.c | 2 +- src/lib/problem_data.c | 57 ++++++++++++++++++++++++-- src/lib/report.c | 109 ++++++++++++++++++------------------------------- 3 files changed, 95 insertions(+), 73 deletions(-) (limited to 'src') 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//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; } -- cgit