[ABRT PATCH 1/2 v2] Make problem_api.c easier to comprehend

Denys Vlasenko dvlasenk at redhat.com
Tue May 21 08:42:19 UTC 2013


In my opinion this verison of the code is simpler.

Instead of having two kinds of callbacks, and *then* hardcoding the action
when they are successful (adding a directory name to the result list),
I combined all these actions into one callback.

This may allow simplification of count_problems_in_dir()
helper (we don't really need to build a list only to free it at once!),
but it's not that easy because we use get_problem_dirs_for_element_in_time()
which is a public function and I don't want to touch public API (yet?).

Version 2: language fix in the comments

Signed-off-by: Denys Vlasenko <dvlasenk at redhat.com>
---
 src/include/problem_api.h |  19 +----
 src/lib/problem_api.c     | 200 +++++++++++++++++++---------------------------
 2 files changed, 86 insertions(+), 133 deletions(-)

diff --git a/src/include/problem_api.h b/src/include/problem_api.h
index 3b1dfd3..b5a952f 100644
--- a/src/include/problem_api.h
+++ b/src/include/problem_api.h
@@ -20,24 +20,11 @@
 #include <glib.h>
 #include <libabrt.h>
 
-/*
- * Structure for simple conditions based on problem fields
- */
-struct problem_condition
-{
-    /* a name of filed required by evaluate function */
-    const char *field_name;
-    /* extra data passed to evaluate function */
-    const void *args;
-    /* evaluate function returning TRUE if condition was passed */
-    bool (*evaluate)(const char *, const void *);
-};
-
 /* Retrieves the list of directories currently used as a problem storage
  * The result must be freed by caller
  * @returns List of strings representing the full path to dirs
-*/
-GList *get_problem_storages();
+ */
+GList *get_problem_storages(void);
 GList *get_problem_dirs_for_uid(uid_t uid, const char *dump_location);
 GList *get_problem_dirs_for_element_in_time(uid_t uid,
                                                       const char *element,
@@ -52,4 +39,4 @@ GList *get_problem_dirs_for_element_in_time(uid_t uid,
  * @since[in]
  * @returns   count of problems
  */
-unsigned int get_problems_count(GList *paths, unsigned long since);
\ No newline at end of file
+unsigned int get_problems_count(GList *paths, unsigned long since);
diff --git a/src/lib/problem_api.c b/src/lib/problem_api.c
index d940767..eebac34 100644
--- a/src/lib/problem_api.c
+++ b/src/lib/problem_api.c
@@ -22,47 +22,23 @@
 #include "problem_api.h"
 
 /*
- * Evaluates a NULL-terminated list of problem conditions as a logical conjunction
+ * Goes through all problems and for problems accessible by caller_uid
+ * calls callback. If callback returns non-0, returns that value.
  */
-static bool problem_condition_evaluate_and(struct dump_dir *dd,
-                                           const struct problem_condition *const *condition)
+static int for_each_problem_in_dir(const char *path,
+                        uid_t caller_uid,
+                        int (*callback)(struct dump_dir *dd, void *arg),
+                        void *arg)
 {
-    /* We stop on the first FALSE condition */
-    while (condition && *condition != NULL)
-    {
-        const struct problem_condition *c = *condition;
-        char *field_data = dd_load_text(dd, c->field_name);
-        bool value = c->evaluate(field_data, c->args);
-        free(field_data);
-        if (!value)
-            return false;
-        ++condition;
-    }
-
-    return true;
-}
-
-/*
- * Goes through all problems and selects only problems accessible by caller_uid and
- * problems for which an and_filter gets TRUE
- *
- * @param condition a NULL-terminated list of problem conditions evaluated
- * as conjunction, can be NULL (means always TRUE)
- */
-static GList* scan_directory(const char *path,
-                             uid_t caller_uid,
-                             const struct problem_condition *const *condition)
-{
-    GList *list = NULL;
-
     DIR *dp = opendir(path);
     if (!dp)
     {
         /* We don't want to yell if, say, $XDG_CACHE_DIR/abrt/spool doesn't exist */
         //perror_msg("Can't open directory '%s'", path);
-        return list;
+        return 0;
     }
 
+    int brk = 0;
     struct dirent *dent;
     while ((dent = readdir(dp)) != NULL)
     {
@@ -80,132 +56,123 @@ static GList* scan_directory(const char *path,
             logmode = 0;
             struct dump_dir *dd = dd_opendir(full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK);
             logmode = sv_logmode;
-            /* or we could just setuid?
-             - but it would require locking, because we want to setuid back before we server another request..
-            */
             if (dd)
             {
-                if (!condition || problem_condition_evaluate_and(dd, condition))
-                {
-                    list = g_list_prepend(list, full_name);
-                    full_name = NULL;
-                }
-                dd_close(dd); //doesn't fail even if dd == NULL
+                brk = callback ? callback(dd, arg) : 0;
+                dd_close(dd);
             }
         }
         free(full_name);
+        if (brk)
+            break;
     }
     closedir(dp);
 
-    /* Why reverse?
+    return brk;
+}
+
+/* get_problem_dirs_for_uid and its helpers */
+
+static int add_dirname_to_GList(struct dump_dir *dd, void *arg)
+{
+    GList **list = arg;
+    *list = g_list_prepend(*list, xstrdup(dd->dd_dirname));
+    return 0;
+}
+
+GList *get_problem_dirs_for_uid(uid_t uid, const char *dump_location)
+{
+    GList *list = NULL;
+    for_each_problem_in_dir(dump_location, uid, add_dirname_to_GList, &list);
+    /*
+     * Why reverse?
      * Because N*prepend+reverse is faster than N*append
      */
     return g_list_reverse(list);
 }
 
-/* Self explaining time interval structure */
-struct time_interval
-{
-    unsigned long from;
-    unsigned long to;
+
+/* get_problem_dirs_for_element_in_time and its helpers */
+
+struct field_and_time_range {
+    GList *list;
+    const char *element;
+    const char *value;
+    unsigned long timestamp_from;
+    unsigned long timestamp_to;
 };
 
-/*
- * A problem condition evaluate function for checking of the TIME field against
- * an allowed interval
- *
- * @param field_data a content from the PID field
- * @param args a pointer to an instance of struct time_interval
- * @return TRUE if a field value is in a specified interval; otherwise FALSE
- */
-static bool time_interval_problem_condition(const char *field_data, const void *args)
+static int add_dirname_to_GList_if_matches(struct dump_dir *dd, void *arg)
 {
-    const struct time_interval *const interval = (const struct time_interval *)args;
-    const time_t timestamp = atol(field_data);
+    struct field_and_time_range *me = arg;
 
-    return interval->from <= timestamp && timestamp <= interval->to;
-}
+    char *field_data;
 
-/*
- * A problem condition evaluate function passed if strings are equal
- *
- * @param field_data a content of a field
- * @param args a checked string
- * @return TRUE if both strings are equal; otherwise FALSE
- */
-static bool equal_string_problem_condition(const char *field_data, const void *args)
-{
-    return !strcmp(field_data, (const char *)args);
-}
+    if (me->element)
+    {
+        field_data = dd_load_text(dd, me->element);
+        int brk = (strcmp(field_data, me->value) != 0);
+        free(field_data);
+        if (brk)
+            return 0;
+    }
 
-GList *get_problem_dirs_for_uid(uid_t uid, const char *dump_location)
-{
-    GList *dirs = scan_directory(dump_location, uid, NULL);
-    return dirs;
+    field_data = dd_load_text(dd, FILENAME_LAST_OCCURRENCE);
+    long val = atol(field_data);
+    free(field_data);
+    if (val < me->timestamp_from || val > me->timestamp_to)
+        return 0;
+
+    me->list = g_list_prepend(me->list, xstrdup(dd->dd_dirname));
+    return 0;
 }
 
 /*
  * Finds problems which were created in the interval
  */
 GList *get_problem_dirs_for_element_in_time(uid_t uid,
-                                                      const char *element,
-                                                      const char *value,
-                                                      unsigned long timestamp_from,
-                                                      unsigned long timestamp_to,
-                                                      const char *dump_location)
+                const char *element,
+                const char *value,
+                unsigned long timestamp_from,
+                unsigned long timestamp_to,
+                const char *dump_location)
 {
-    struct timeval tv;
-    /* use the current time if timestamp_to is 0 */
-    if (timestamp_to == 0) {
-        gettimeofday(&tv, NULL);
-        timestamp_to = tv.tv_sec;
-    }
-
-
-    const struct problem_condition elementc = {
-        .field_name = element,
-        .args = value,
-        .evaluate = equal_string_problem_condition,
+    if (timestamp_to == 0)
+        timestamp_to = time(NULL);
+
+    struct field_and_time_range me = {
+        .list = NULL,
+        .element = element,
+        .value = value,
+        .timestamp_from = timestamp_from,
+        .timestamp_to = timestamp_to,
     };
 
-    const struct time_interval interval = {
-        .from = timestamp_from,
-        .to = timestamp_to,
-    };
-
-    const struct problem_condition timec = {
-        .field_name = FILENAME_LAST_OCCURRENCE,
-        .args = &interval,
-        .evaluate = time_interval_problem_condition
-    };
+    for_each_problem_in_dir(dump_location, uid, add_dirname_to_GList_if_matches, &me);
 
-    const struct problem_condition *const condition[] = {
-        &timec,
-        element ? &elementc : NULL,
-        NULL
-    };
-    GList *dirs = scan_directory(dump_location, uid, condition);
-    return dirs;
+    return g_list_reverse(me.list);
 }
 
-GList *get_problem_storages()
+/* get_problem_storages */
+
+GList *get_problem_storages(void)
 {
     GList *pths = NULL;
     load_abrt_conf();
     pths = g_list_append(pths, xstrdup(g_settings_dump_location));
-    //no needed, we don't steal directories anymore
+    //not needed, we don't steal directories anymore
     pths = g_list_append(pths, concat_path_file(g_get_user_cache_dir(), "abrt/spool"));
     free_abrt_conf_data();
 
     return pths;
 }
 
+/* get_problems_count and its helpers */
 
 typedef struct problem_count_info {
     int problem_count;
     unsigned long since;
     unsigned long until;
-
 } problem_count_info_t;
 
 static void count_problems_in_dir(char *path, gpointer problem_counter)
@@ -223,18 +190,17 @@ unsigned int get_problems_count(GList *paths, unsigned long since)
 
     if (paths == NULL)
     {
-        pths = get_problem_storages();
+        paths = pths = get_problem_storages();
     }
 
     problem_count_info_t pci;
-
     pci.problem_count = 0;
     pci.since = since;
-    pci.until = 0;
+    pci.until = time(NULL);
 
-    g_list_foreach(paths ? paths : pths, (GFunc)count_problems_in_dir, &pci);
+    g_list_foreach(paths, (GFunc)count_problems_in_dir, &pci);
 
     list_free_with_free(pths);
 
     return pci.problem_count;
-}
\ No newline at end of file
+}
-- 
1.8.1.4



More information about the Crash-catcher mailing list