[PATCH v2] rhbz#622773 : abrt-dbus new high-level problem query functions

[Jakub Filak]

Signed-off-by: Jakub Filak <jfilak(a)redhat.com&gt;
---
 src/dbus/abrt-dbus.c |  209 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 205 insertions(+), 4 deletions(-)

diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c
index 6bdca48..cfbc086 100644
--- a/src/dbus/abrt-dbus.c
+++ b/src/dbus/abrt-dbus.c
@@ -38,6 +38,14 @@ static const gchar introspection_xml[] =
   "    <method name='DeleteProblem'>"
   "      <arg type='as' name='problem_dir'
direction='in'/>"
   "    </method>"
+  "    <method name='FindProblemByElementInTimeRange'>"
+  "      <arg type='s' name='element'
direction='in'/>"
+  "      <arg type='s' name='value'
direction='in'/>"
+  "      <arg type='t' name='timestamp_from'
direction='in'/>"
+  "      <arg type='t' name='timestamp_to'
direction='in'/>"
+  "      <arg type='b' name='all_users'
direction='in'/>"
+  "      <arg type='as' name='response'
direction='out'/>"
+  "    </method>"
   "    <method name='Quit' />"
   "  </interface>"
   "</node>";
@@ -154,7 +162,50 @@ static int dir_accessible_by_uid(const char* dir_path, uid_t uid)
     return 0;
 }
 
-static GList* scan_directory(const char *path, uid_t caller_uid)
+/*
+ * 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 *);
+};
+
+/*
+ * Evaluates a NULL-terminated list of problem conditions as a logical conjunction
+ */
+static bool problem_condition_evaluate_and(struct dump_dir *dd,
+                                           const struct problem_condition *const
*condition)
+{
+    /* We stop on the first FALSE condition */
+    while (*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;
 
@@ -189,8 +240,11 @@ static GList* scan_directory(const char *path, uid_t caller_uid)
             */
             if (dd && (caller_uid == 0 || statbuf.st_mode & S_IROTH ||
uid_in_group(caller_uid, statbuf.st_gid)))
             {
-                list = g_list_prepend(list, full_name);
-                full_name = NULL;
+                if (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
         }
@@ -204,9 +258,120 @@ static GList* scan_directory(const char *path, uid_t caller_uid)
     return g_list_reverse(list);
 }
 
+/* Self explaining time interval structure */
+struct time_interval
+{
+    unsigned long from;
+    unsigned long 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)
+{
+    if (!field_data)
+        return FALSE;
+
+    const struct time_interval *const interval = (const struct time_interval *)args;
+    const time_t timestamp = atol(field_data);
+
+    return interval->from <= timestamp && timestamp <= interval->to;
+}
+
+/*
+ * 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 (field_data == NULL && args == NULL)
+            || (field_data != NULL && !strcmp(field_data, (const char *)args));
+}
+
+/*
+ * A problem condition evaluate function passed if args string is substring
+ * of element field data
+ *
+ * @param field_data a content of a field
+ * @param args a checked string
+ * @return TRUE if args is substring of field_data; otherwise FALSE
+ */
+static bool substring_problem_condition(const char *field_data, const void* args)
+{
+    /* BEWARE : formally empty string IS ALWAYS substring of any string */
+    /*          but it is NOT VALID in our case */
+    return field_data != NULL && strstr(field_data, (const char *)args);
+}
+
 static GVariant *get_problem_dirs_for_uid(uid_t uid, const char *dump_location)
 {
-    GList *dirs = scan_directory(dump_location, uid);
+    GList *dirs = scan_directory(dump_location, uid, NULL);
+
+    return variant_from_string_list(dirs);
+}
+
+/*
+ * Provides basic initialization according to required element
+ *
+ * @param condition a problem condition structure
+ * @param element a name of problem dir field
+ * @param value a string for which we are looking for
+ */
+static void initialize_element_condition(struct problem_condition *condition,
+                                         const char *element,
+                                         const char *value)
+{
+    condition->field_name = element;
+    condition->args = value;
+
+    /* use substring evaluate function for CGROUP element */
+    if (strcmp(element, FILENAME_CGROUP) == 0)
+        condition->evaluate = substring_problem_condition;
+    else
+        /* an evaluate function is string equality by default */
+        condition->evaluate = equal_string_problem_condition;
+}
+
+/*
+ * Finds problems with the specified element and which were created in the interval
+ */
+static GVariant *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 struct time_interval interval = {
+        .from = timestamp_from,
+        .to = timestamp_to
+    };
+
+    const struct problem_condition timec = {
+        .field_name = FILENAME_TIME,
+        .args = &interval,
+        .evaluate = time_interval_problem_condition
+    };
+
+    struct problem_condition elementc;
+    initialize_element_condition(&elementc, element, value);
+
+    const struct problem_condition *const condition[] = {
+        &elementc,
+        &timec,
+        NULL
+    };
+
+    GList *dirs = scan_directory(dump_location, uid, condition);
 
     return variant_from_string_list(dirs);
 }
@@ -454,6 +619,42 @@ static void handle_method_call(GDBusConnection *connection,
         return;
     }
 
+    else if (g_strcmp0(method_name, "FindProblemByElementInTimeRange") == 0)
+    {
+        const char* element;
+        const char* value;
+        unsigned long timestamp_from;
+        unsigned long timestamp_to;
+        bool all;
+
+        g_variant_get(parameters, "(ssttb)", &element, &value,
&timestamp_from, &timestamp_to, &all);
+
+        if (strlen(element) == 0)
+        {
+            g_dbus_method_invocation_return_dbus_error(invocation,
+                                                      
"org.freedesktop.problems.InvalidElement",
+                                                       _("Empty Element
Name"));
+            return;
+        }
+
+        if (all && polkit_check_authorization_dname(caller,
"org.freedesktop.problems.getall") == PolkitYes)
+            caller_uid = 0;
+
+        /* FIXME : Temporary until we decide how to handle this */
+        if (!g_settings_dump_location)
+            g_settings_dump_location = (char*)"/var/spool/abrt";
+
+        response = get_problem_dirs_for_element_in_time(caller_uid,
+                                                        element,
+                                                        value,
+                                                        timestamp_from,
+                                                        timestamp_to,
+                                                        g_settings_dump_location);
+
+        g_dbus_method_invocation_return_value(invocation, response);
+        return;
+    }
+
     else if (g_strcmp0(method_name, "Quit") == 0)
     {
         VERB1 log("Quit");
-- 
1.7.7.6

[Denys Vlasenko]

On 05/30/2012 12:37 PM, Jakub Filak wrote:
>  +static bool problem_condition_evaluate_and(struct dump_dir *dd,
>  +                                           const struct problem_condition *const
> *condition)
>  +{
>  +    /* We stop on the first FALSE condition */
>  +    while (*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); 
Note that field_data is never NULL... (see below).

>  +static bool time_interval_problem_condition(const char *field_data,
> const void* args)
>  +{
>  +    if (!field_data)
>  +        return FALSE; 
Never happens.

>  +    const struct time_interval *const interval = (const struct
> time_interval *)args;
>  +    const time_t timestamp = atol(field_data);
>  +
>  +    return interval->from<= timestamp && timestamp <= interval->to;
Why timespamp == 0 is treated specially?
If there is a reason, it should be explained in a comment somewhere.

>  +static bool equal_string_problem_condition(const char *field_data,
> const void* args)
>  +{
>  +    return (field_data == NULL && args == NULL) 
Never happens.

>  +            || (field_data != NULL 
Always true

                         && !strcmp(field_data, (const char *)args));


>  +static bool substring_problem_condition(const char *field_data,
> const void* args)
>  +{
>  +    /* BEWARE : formally empty string IS ALWAYS substring of any string */
>  +    /*          but it is NOT VALID in our case */
>  +    return field_data != NULL 
field_data is always !NULL

                           && strstr(field_data, (const char *)args);


>  +static void initialize_element_condition(struct problem_condition
> *condition,
>  +                                         const char *element,
>  +                                         const char *value)
>  +{
>  +    condition->field_name = element;
>  +    condition->args = value;
>  +
>  +    /* use substring evaluate function for CGROUP element */
>  +    if (strcmp(element, FILENAME_CGROUP) == 0)
>  +        condition->evaluate = substring_problem_condition;
>  +    else
>  +        /* an evaluate function is string equality by default */
>  +        condition->evaluate = equal_string_problem_condition;
>  +} 
I'm not that familiar with cgroups. Why we do substring match?
How cgroup names look like? Maybe add a comment with an example...


>  +        g_variant_get(parameters,
> "(ssttb)",&element,&value,&timestamp_from,&timestamp_to,&all);
>  +
>  +        if (strlen(element) == 0) 
"if (element[0])" is more efficient check.

>  +        {
>  +            g_dbus_method_invocation_return_dbus_error(invocation,
>  +                                                      
> "org.freedesktop.problems.InvalidElement",
>  +                                                       _("Empty Element
> Name"));
>  +            return;
>  +        } 
I don't understand why you don't allow empty names.
Granted, they don't make sense, but so do names like "\/\/\\//\陰莖增大增***",
but we don't check for that, right?
I propose not checking for "".

>  +
>  +        if (all&&  polkit_check_authorization_dname(caller,
> "org.freedesktop.problems.getall") == PolkitYes)
>  +            caller_uid = 0;
>  +
>  +        /* FIXME : Temporary until we decide how to handle this */
>  +        if (!g_settings_dump_location)
>  +            g_settings_dump_location = (char*)"/var/spool/abrt"; 
g_settings_dump_location must be a malloced string, or else:

void free_abrt_conf_data()
{
     free(g_settings_sWatchCrashdumpArchiveDir);
     g_settings_sWatchCrashdumpArchiveDir = NULL;

     free(g_settings_dump_location);  <============= BOOM
     g_settings_dump_location = NULL;
}

>  +
>  +        response = get_problem_dirs_for_element_in_time(caller_uid,
>  +                                                        element,
>  +                                                        value,
>  +                                                        timestamp_from,
>  +                                                        timestamp_to,
>  +                                                        g_settings_dump_location);
You can simply add that bit here instead:

g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt"

-- 
vda

[Nikola Pajkovsky]

Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes:

>  On 05/30/2012 12:37 PM, Jakub Filak wrote:
> > +static bool problem_condition_evaluate_and(struct dump_dir *dd,
> > +                                           const struct problem_condition *const
> *condition)
> > +{
> > +    /* We stop on the first FALSE condition */
> > +    while (*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);
>
>  Note that field_data is never NULL... (see below).
>
> > +static bool time_interval_problem_condition(const char *field_data, const void*
> args)
> > +{
> > +    if (!field_data)
> > +        return FALSE;
>
>  Never happens.
>
> > +    const struct time_interval *const interval = (const struct time_interval
> *)args;
> > +    const time_t timestamp = atol(field_data);
> > +
> > +    return interval->from<= timestamp && timestamp <=
> interval->to;
>
>  Why timespamp == 0 is treated specially?
>  If there is a reason, it should be explained in a comment somewhere.
>
> > +static bool equal_string_problem_condition(const char *field_data, const void*
> args)
> > +{
> > +    return (field_data == NULL && args == NULL)
>
>  Never happens.
>
> > +            || (field_data != NULL
>
>  Always true
>
>                          && !strcmp(field_data, (const char *)args));
>
>
> > +static bool substring_problem_condition(const char *field_data, const void* args)
> > +{
> > +    /* BEWARE : formally empty string IS ALWAYS substring of any string */
> > +    /*          but it is NOT VALID in our case */
> > +    return field_data != NULL
>
>  field_data is always !NULL
>
>                            && strstr(field_data, (const char *)args);
>
>
> > +static void initialize_element_condition(struct problem_condition *condition,
> > +                                         const char *element,
> > +                                         const char *value)
> > +{
> > +    condition->field_name = element;
> > +    condition->args = value;
> > +
> > +    /* use substring evaluate function for CGROUP element */
> > +    if (strcmp(element, FILENAME_CGROUP) == 0)
> > +        condition->evaluate = substring_problem_condition;
> > +    else
> > +        /* an evaluate function is string equality by default */
> > +        condition->evaluate = equal_string_problem_condition;
> > +}
>
>  I'm not that familiar with cgroups. Why we do substring match?
>  How cgroup names look like? Maybe add a comment with an example...
>
>
> > +        g_variant_get(parameters,
> "(ssttb)",&element,&value,&timestamp_from,&timestamp_to,&all);
> > +
> > +        if (strlen(element) == 0)
>
>  "if (element[0])" is more efficient check.
>
> > +        {
> > +            g_dbus_method_invocation_return_dbus_error(invocation,
> > +                                                      
> "org.freedesktop.problems.InvalidElement",
> > +                                                       _("Empty Element
> Name"));
> > +            return;
> > +        }
>
>  I don't understand why you don't allow empty names.
>  Granted, they don't make sense, but so do names like "\/\/\\//\陰莖增大增***",
>  but we don't check for that, right?
>  I propose not checking for "".
>
> > +
> > +        if (all&&  polkit_check_authorization_dname(caller,
> "org.freedesktop.problems.getall") == PolkitYes)
> > +            caller_uid = 0;
> > +
> > +        /* FIXME : Temporary until we decide how to handle this */
> > +        if (!g_settings_dump_location)
> > +            g_settings_dump_location = (char*)"/var/spool/abrt";
>
>  g_settings_dump_location must be a malloced string, or else: 
g_settings_dump_location is always set to location from config or
malloc'ed "/var/spool/abrt" string if abrt_load_conf() is called

snip from abrt/src/lib/abrt_conf.c:65

    g_hash_table_remove(settings, "DumpLocation");
    }
    else
        g_settings_dump_location = xstrdup("/var/spool/abrt"); <===== HERE

    GHashTableIter ite

>  void free_abrt_conf_data()
>  {
>      free(g_settings_sWatchCrashdumpArchiveDir);
>      g_settings_sWatchCrashdumpArchiveDir = NULL;
>
>      free(g_settings_dump_location);  <============= BOOM
>      g_settings_dump_location = NULL;
>  }
>
> > +
> > +        response = get_problem_dirs_for_element_in_time(caller_uid,
> > +                                                        element,
> > +                                                        value,
> > +                                                        timestamp_from,
> > +                                                        timestamp_to,
> > +                                                        g_settings_dump_location);
>
>  You can simply add that bit here instead:
>
>  g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt" 
-- 
Nikola

[Jakub Filak]

On Wednesday 30 of May 2012 15:14:23 Nikola Pajkovsky wrote:
>  Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes:
>  > On 05/30/2012 12:37 PM, Jakub Filak wrote:
>  >> +static bool problem_condition_evaluate_and(struct dump_dir *dd,
>  >> +                                           const struct
>  >> problem_condition *const *condition) +{
>  >> +    /* We stop on the first FALSE condition */
>  >> +    while (*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);
>  > 
>  > Note that field_data is never NULL... (see below).
>  > 
>  >> +static bool time_interval_problem_condition(const char *field_data,
>  >> const void* args) +{
>  >> +    if (!field_data)
>  >> +        return FALSE;
>  > 
>  > Never happens.
>  > 
>  >> +    const struct time_interval *const interval = (const struct
>  >> time_interval *)args; +    const time_t timestamp = atol(field_data);
>  >> +
>  >> +    return interval->from<= timestamp && timestamp <=
> interval->to;
>  > 
>  > Why timespamp == 0 is treated specially?
>  > If there is a reason, it should be explained in a comment somewhere.
>  > 
>  >> +static bool equal_string_problem_condition(const char *field_data, const
>  >> void* args) +{
>  >> +    return (field_data == NULL && args == NULL)
>  > 
>  > Never happens.
>  > 
>  >> +            || (field_data != NULL
>  > 
>  > Always true
>  > 
>  >                         && !strcmp(field_data, (const char *)args));
>  >> 
>  >> +static bool substring_problem_condition(const char *field_data, const
>  >> void* args) +{
>  >> +    /* BEWARE : formally empty string IS ALWAYS substring of any string
>  >> */
>  >> +    /*          but it is NOT VALID in our case */
>  >> +    return field_data != NULL
>  > 
>  > field_data is always !NULL
>  > 
>  >                           && strstr(field_data, (const char *)args);
>  >> 
>  >> +static void initialize_element_condition(struct problem_condition
>  >> *condition, +                                         const char
>  >> *element,
>  >> +                                         const char *value)
>  >> +{
>  >> +    condition->field_name = element;
>  >> +    condition->args = value;
>  >> +
>  >> +    /* use substring evaluate function for CGROUP element */
>  >> +    if (strcmp(element, FILENAME_CGROUP) == 0)
>  >> +        condition->evaluate = substring_problem_condition;
>  >> +    else
>  >> +        /* an evaluate function is string equality by default */
>  >> +        condition->evaluate = equal_string_problem_condition;
>  >> +}
>  > 
>  > I'm not that familiar with cgroups. Why we do substring match?
>  > How cgroup names look like? Maybe add a comment with an example...
>  > 
>  >> +        g_variant_get(parameters,
>  >>
> "(ssttb)",&element,&value,&timestamp_from,&timestamp_to,&all);
> +
>  >> +        if (strlen(element) == 0)
>  > 
>  > "if (element[0])" is more efficient check.
>  > 
>  >> +        {
>  >> +            g_dbus_method_invocation_return_dbus_error(invocation,
>  >> +                                                      
>  >> "org.freedesktop.problems.InvalidElement", +                          
>  
>  >>                           _("Empty Element Name")); +           
> return;
>  >> +        }
>  > 
>  > I don't understand why you don't allow empty names.
>  > Granted, they don't make sense, but so do names like
> "\/\/\\//\陰莖增大增***",
>  > but we don't check for that, right?
>  > I propose not checking for "".
>  > 
>  >> +
>  >> +        if (all&&  polkit_check_authorization_dname(caller,
>  >> "org.freedesktop.problems.getall") == PolkitYes) +           
> caller_uid
>  >> = 0;
>  >> +
>  >> +        /* FIXME : Temporary until we decide how to handle this */
>  >> +        if (!g_settings_dump_location)
>  >> +            g_settings_dump_location = (char*)"/var/spool/abrt";
>  > 
>  > g_settings_dump_location must be a malloced string, or else:
>  g_settings_dump_location is always set to location from config or
>  malloc'ed "/var/spool/abrt" string if abrt_load_conf() is called
>  
>  snip from abrt/src/lib/abrt_conf.c:65
>  
>      g_hash_table_remove(settings, "DumpLocation");
>      }
>      else
>          g_settings_dump_location = xstrdup("/var/spool/abrt"); <===== HERE
>  
>      GHashTableIter ite
>  
>  > void free_abrt_conf_data()
>  > {
>  > 
>  >     free(g_settings_sWatchCrashdumpArchiveDir);
>  >     g_settings_sWatchCrashdumpArchiveDir = NULL;
>  >     
>  >     free(g_settings_dump_location);  <============= BOOM
>  >     g_settings_dump_location = NULL;
>  > 
>  > } 
abrt_load_conf() nor free_abrt_conf_data() are never called in abrt-dbus.c
file

Therefore I wrote the following comment:
/* FIXME : Temporary until we decide how to handle this */

I'm not sure why we don't load ABRT configuration.

So, I'm going to use this solution:
g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt"

>  > 
>  >> +
>  >> +        response = get_problem_dirs_for_element_in_time(caller_uid,
>  >> +                                                        element,
>  >> +                                                        value,
>  >> +                                                        timestamp_from,
>  >> +                                                        timestamp_to,
>  >> +                                                       
>  >> g_settings_dump_location);> 
>  > You can simply add that bit here instead:
>  > 
>  > g_settings_dump_location ? g_settings_dump_location : "/var/spool/abrt"