run_event.c: fix "EVENT=post-create component=mypkg doesn't work" bug. Closes bz#531365

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

2 files changed, 201 insertions, 235 deletions

diff --git a/src/include/report/run_event.h b/src/include/report/run_event.h
index 12528920..9726b643 100644
--- a/src/include/report/run_event.h
+++ b/src/include/report/run_event.h
@@ -41,7 +41,7 @@ struct run_event_state {
     void *logging_param;
 
     /* Internal data for async command execution */
-    GList *commands;
+    GList *rule_list;
     pid_t command_pid;
     int command_out_fd;
 };
diff --git a/src/lib/run_event.c b/src/lib/run_event.c
index 883dc6d0..f8aaf28e 100644
--- a/src/lib/run_event.c
+++ b/src/lib/run_event.c
@@ -49,22 +49,54 @@ void free_run_event_state(struct run_event_state *state)
  * After cmd1, should we execute cmd2 if element foo disappeared?
  * After cmd1/2, should we execute cmd3 if element foo changed value to baz?
  *
- * So far, we read entire config file and select a list of commands to execute,
+ * We used to read entire config file and select a list of commands to execute,
  * checking all conditions in the beginning. It is a bit more simple to code up.
- * But we may want to change it later. Therefore list of commands machinery
- * is encapsulated in struct run_event_state and public async API:
+ *
+ * This proved to be bad for use cases where, for example, post-create rule
+ * for a specified package needs to run:
+ *
+ * EVENT=post-create
+ *      abrt-action-save-package-data
+ * EVENT=post-create component=mypkg
+ *      my_handling
+ *
+ * Problem here is that "component" element is created by
+ * abrt-action-save-package-data! Pre-selecting rules excludes second rule.
+ *
+ * Now we read entire config but do NOT select commands to execute,
+ * we check conditions of every next rule *directly before its execution*.
+ *
+ * Anyway, list of commands machinery is encapsulated in struct run_event_state,
+ * and public async API:
  *      prepare_commands(state, dir, event);
  *      spawn_next_command(state, dir, event);
  *      free_commands(state);
- * does not expose it.
+ * does not expose the way we select rules to execute.
  */
+struct rule {
+    GList *conditions;
+    char *command; /* never NULL */
+};
+
+static void free_rule_list(GList *rule_list)
+{
+    while (rule_list)
+    {
+        struct rule *cur_rule = rule_list->data;
+        list_free_with_free(cur_rule->conditions);
+        free(cur_rule->command);
+        free(cur_rule);
+
+        GList *next = rule_list->next;
+        g_list_free_1(rule_list);
+        rule_list = next;
+    }
+}
 
 /* Stop-gap measure against infinite recursion */
 #define MAX_recursion_depth 32
 
-static GList *load_event_config(GList *list,
-                const char *dump_dir_name,
-                const char *event,
+static GList *load_rule_list(GList *rule_list,
                 const char *conf_file_name,
                 unsigned recursion_depth
 ) {
@@ -72,14 +104,14 @@ static GList *load_event_config(GList *list,
     if (!conffile)
     {
         error_msg("Can't open '%s'", conf_file_name);
-        return list;
+        return rule_list;
     }
 
-    /* Read, match, and remember commands to execute */
-    struct dump_dir *dd = NULL;
+    /* Read and remember rules */
     char *next_line = xmalloc_fgetline(conffile);
     while (next_line)
     {
+        /* Read and concatenate all lines in a rule */
         char *line = next_line;
         while (1)
         {
@@ -92,18 +124,18 @@ static GList *load_event_config(GList *list,
             free(next_line);
         }
 
-        /* Line has form: [VAR=VAL]... PROG [ARGS] */
         char *p = skip_whitespace(line);
         if (*p == '\0' || *p == '#')
             goto next_line; /* empty or comment line, skip */
 
         //VERB3 log("%s: line '%s'", __func__, p);
 
+        /* Handle "include" directive */
         if (recursion_depth < MAX_recursion_depth
          && strncmp(p, "include", strlen("include")) == 0
          && isblank(p[strlen("include")])
         ) {
-            /* include GLOB_PATTERN */
+            /* "include GLOB_PATTERN" */
             p = skip_whitespace(p + strlen("include"));
 
             const char *last_slash;
@@ -130,7 +162,7 @@ static GList *load_event_config(GList *list,
             if (name) while (*name)
             {
                 //VERB3 log("%s: recursing into '%s'", __func__, *name);
-                list = load_event_config(list, dump_dir_name, event, *name, recursion_depth + 1);
+                rule_list = load_rule_list(rule_list, *name, recursion_depth + 1);
                 //VERB3 log("%s: returned from '%s'", __func__, *name);
                 name++;
             }
@@ -138,76 +170,144 @@ static GList *load_event_config(GList *list,
             goto next_line;
         }
 
+        /* Rule has form: [VAR=VAL]... PROG [ARGS] */
+        struct rule *cur_rule = xzalloc(sizeof(*cur_rule));
+
         while (1) /* word loop */
         {
             char *end_word = skip_non_whitespace(p);
-            char *next_word = skip_whitespace(end_word);
-
-            /* *end_word = '\0'; - BUG, truncates command */
 
             /* If there is no '=' in this word... */
             char *line_val = strchr(p, '=');
             if (!line_val || line_val >= end_word)
                 break; /* ...we found the start of a command */
 
-            *end_word = '\0';
+            cur_rule->conditions = g_list_append(cur_rule->conditions, xstrndup(p, end_word - p));
+
+            /* Go to next word */
+            p = skip_whitespace(end_word);
+        } /* end of word loop */
+
+        VERB1 log("Adding '%s'", p);
+        cur_rule->command = xstrdup(p);
+
+        rule_list = g_list_append(rule_list, cur_rule);
 
-            /* Current word has VAR=VAL form. line_val => VAL */
-            *line_val++ = '\0';
+ next_line:
+        free(line);
+    } /* end of line loop */
 
-            const char *real_val;
-            char *malloced_val = NULL;
+    fclose(conffile);
 
-            /* Is it EVENT? */
-            if (strcmp(p, "EVENT") == 0)
-                real_val = event;
+    return rule_list;
+}
+
+/* Note: may return NULL but leave list non-NULL.
+ * This happens on error, such as "dump dir can't be opened"
+ */
+static char* pop_next_command(GList **pp_rule_list,
+        char **pp_event_name,    /* reports EVENT value thru this, if not NULL on entry */
+        struct dump_dir **pp_dd, /* use *pp_dd for access to dump dir, if non-NULL */
+        const char *dump_dir_name,
+        const char *pfx,
+        unsigned pfx_len
+)
+{
+    char *command = NULL;
+    struct dump_dir *dd = pp_dd ? *pp_dd : NULL;
+
+    GList *rule_list = *pp_rule_list;
+    while (rule_list)
+    {
+        struct rule *cur_rule = rule_list->data;
+
+        GList *condition = cur_rule->conditions;
+        while (condition)
+        {
+            const char *cond_str = condition->data;
+            const char *eq_sign = strchr(cond_str, '=');
+
+            /* Is it "EVENT=foo"? */
+            if (strncmp(cond_str, "EVENT=", 6) == 0)
+            {
+                if (strncmp(eq_sign + 1, pfx, pfx_len) != 0)
+                    goto next_rule; /* prefix doesn't match */
+                if (pp_event_name)
+                {
+                    free(*pp_event_name);
+                    *pp_event_name = xstrdup(eq_sign + 1);
+                }
+            }
             else
             {
-                /* Get this name from dump dir */
+                /* Read from dump dir and compare */
                 if (!dd)
                 {
-                    dd = dd_opendir(dump_dir_name, DD_OPEN_READONLY);
+                    /* Without dir to match, we assume match for all conditions */
+                    if (!dump_dir_name)
+                        goto next_cond;
+                    dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
                     if (!dd)
                     {
-                        free(line);
-                        free(next_line);
-                        goto stop; /* error (note: dd_opendir logged error msg) */
+                        goto ret; /* error (note: dd_opendir logged error msg) */
                     }
                 }
-                real_val = malloced_val = dd_load_text_ext(dd, p, DD_FAIL_QUIETLY_ENOENT);
-            }
+                char *var_name = xstrndup(cond_str, eq_sign - cond_str);
+                char *real_val = dd_load_text_ext(dd, var_name, DD_FAIL_QUIETLY_ENOENT);
+                free(var_name);
+                int vals_differ = strcmp(real_val, eq_sign + 1);
+                free(real_val);
 
-            /* Does VAL match? */
-            if (strcmp(real_val, line_val) != 0)
-            {
-                //VERB3 log("var '%s': '%.*s'!='%s', skipping line",
-                //        p,
-                //        (int)(strchrnul(real_val, '\n') - real_val), real_val,
-                //        line_val);
-                free(malloced_val);
-                goto next_line; /* no */
+                /* Do values match? */
+                if (vals_differ) /* no */
+                {
+                    //VERB3 log("var '%s': '%.*s'!='%s', skipping line",
+                    //        p,
+                    //        (int)(strchrnul(real_val, '\n') - real_val), real_val,
+                    //        eq_sign);
+                    goto next_rule;
+                }
             }
-            free(malloced_val);
+ next_cond:
+            /* We are here if current condition is satisfied */
 
-            /* Go to next word */
-            p = next_word;
-        } /* end of word loop */
+            condition = condition->next;
+        }
+        /* We are here if all conditions are satisfied */
 
-        /* We found matching line, remember its command */
-        VERB1 log("Adding '%s'", p);
-        overlapping_strcpy(line, p);
-        list = g_list_append(list, line);
-        continue;
+        command = cur_rule->command;
 
- next_line:
-        free(line);
-    } /* end of line loop */
+ next_rule:
+        *pp_rule_list = rule_list->next;
+        g_list_free_1(rule_list);
 
- stop:
-    dd_close(dd);
-    fclose(conffile);
+        list_free_with_free(cur_rule->conditions);
+        /*free(cur_rule->command); - WRONG! we might be returning it! */
+        if (command)
+        {
+            free(cur_rule);
+            break;
+        }
+        free(cur_rule->command); /* _now_ it is ok */
+        free(cur_rule);
+
+        rule_list = *pp_rule_list;
+    } /* while (rule_list) */
 
-    return list;
+ ret:
+    if (pp_dd)
+        *pp_dd = dd;
+    else
+        dd_close(dd);
+    return command;
+}
+
+void free_commands(struct run_event_state *state)
+{
+    free_rule_list(state->rule_list);
+    state->rule_list = NULL;
+    state->command_out_fd = -1;
+    state->command_pid = 0;
 }
 
 int prepare_commands(struct run_event_state *state,
@@ -218,27 +318,22 @@ int prepare_commands(struct run_event_state *state,
 
     state->children_count = 0;
 
-    GList *commands = load_event_config(NULL, dump_dir_name, event, CONF_DIR"/abrt_event.conf", /*recursion_depth:*/ 0);
-    state->commands = commands;
-    return commands != NULL;
-}
-
-void free_commands(struct run_event_state *state)
-{
-    list_free_with_free(state->commands);
-    state->commands = NULL;
-    state->command_out_fd = -1;
-    state->command_pid = 0;
+    GList *rule_list = load_rule_list(NULL, CONF_DIR"/abrt_event.conf", /*recursion_depth:*/ 0);
+    state->rule_list = rule_list;
+    return rule_list != NULL;
 }
 
-/* event parameter is unused for now,
- * but may be needed if we change implementation later
- */
 int spawn_next_command(struct run_event_state *state,
                 const char *dump_dir_name,
                 const char *event
 ) {
-    if (!state->commands)
+    char *cmd = pop_next_command(&state->rule_list,
+                NULL,          /* don't return event_name */
+                NULL,          /* NULL &dd: we match by... */
+                dump_dir_name, /* ...dirname */
+                event, strlen(event)+1 /* for this event name exactly (not prefix) */
+    );
+    if (!cmd)
         return -1;
 
     /* We count it even if fork fails. The counter isn't meant
@@ -247,7 +342,6 @@ int spawn_next_command(struct run_event_state *state,
      */
     state->children_count++;
 
-    char *cmd = state->commands->data;
     VERB1 log("Executing '%s'", cmd);
 
     /* Export some useful environment variables for children */
@@ -281,14 +375,13 @@ int spawn_next_command(struct run_event_state *state,
 
     free(env_vec[0]);
     free(env_vec[1]);
-
-    state->commands = g_list_remove(state->commands, cmd);
+    free(cmd);
 
     return 0;
 }
 
 
-/* Syncronous command execution:
+/* Synchronous command execution:
  */
 int run_event_on_dir_name(struct run_event_state *state,
                 const char *dump_dir_name,
@@ -357,178 +450,51 @@ int run_event_on_problem_data(struct run_event_state *state, problem_data_t *dat
         load_problem_data_from_dump_dir(data, dd);
         dd_delete(dd);
     }
+
     return r;
 }
 
+char *list_possible_events(struct dump_dir *dd, const char *dump_dir_name, const char *pfx)
+{
+    struct strbuf *result = strbuf_new();
 
-/* TODO: very similar to run_event_helper, try to combine into one fn? */
-static int list_possible_events_helper(struct strbuf *result,
-                struct dump_dir *dd,
-                const char *dump_dir_name,
-                const char *pfx,
-                const char *conf_file_name
-) {
-    FILE *conffile = fopen(conf_file_name, "r");
-    if (!conffile)
-    {
-        error_msg("Can't open '%s'", conf_file_name);
-        return 0;
-    }
-
-    /* We check "dump_dir_name == NULL" later.
-     * Prevent the possibility that both dump_dir_name
-     * and dd are non-NULL (which does not make sense)
-     */
-    if (dd)
-        dump_dir_name = NULL;
+    GList *rule_list = load_rule_list(NULL, CONF_DIR"/abrt_event.conf", /*recursion_depth:*/ 0);
 
-    int error = 0;
     unsigned pfx_len = strlen(pfx);
-    char *next_line = xmalloc_fgetline(conffile);
-    while (next_line)
+    for (;;)
     {
-        char *line = next_line;
-        while (1)
-        {
-            next_line = xmalloc_fgetline(conffile);
-            if (!next_line || !isblank(next_line[0]))
-                break;
-            char *old_line = line;
-            line = xasprintf("%s\n%s", line, next_line);
-            free(old_line);
-            free(next_line);
-        }
-
-        /* Line has form: [VAR=VAL]... PROG [ARGS] */
-        char *p = skip_whitespace(line);
-        if (*p == '\0' || *p == '#')
-            goto next_line; /* empty or comment line, skip */
-
-        //VERB3 log("%s: line '%s'", __func__, p);
-
-        if (strncmp(p, "include", strlen("include")) == 0 && isblank(p[strlen("include")]))
-        {
-            /* include GLOB_PATTERN */
-            p = skip_whitespace(p + strlen("include"));
-
-            const char *last_slash;
-            char *name_to_glob;
-            if (*p != '/'
-             && (last_slash = strrchr(conf_file_name, '/')) != NULL
-            )
-                /* GLOB_PATTERN is relative, and this include is in path/to/file.conf
-                 * Construct path/to/GLOB_PATTERN:
-                 */
-                name_to_glob = xasprintf("%.*s%s", (int)(last_slash - conf_file_name + 1), conf_file_name, p);
-            else
-                /* Either GLOB_PATTERN is absolute, or this include is in file.conf
-                 * (no slashes in its name). Use unchanged GLOB_PATTERN:
-                 */
-                name_to_glob = xstrdup(p);
-
-            glob_t globbuf;
-            memset(&globbuf, 0, sizeof(globbuf));
-            //VERB3 log("%s: globbing '%s'", __func__, name_to_glob);
-            glob(name_to_glob, 0, NULL, &globbuf);
-            free(name_to_glob);
-            char **name = globbuf.gl_pathv;
-            if (name) while (*name)
-            {
-                //VERB3 log("%s: recursing into '%s'", __func__, *name);
-                error = list_possible_events_helper(result, dd, dump_dir_name, pfx, *name);
-                //VERB3 log("%s: returned from '%s'", __func__, *name);
-                if (error)
-                    break;
-                name++;
-            }
-            globfree(&globbuf);
-            goto next_line;
-        }
+        /* Retrieve each cmd, and fetch its EVENT=foo value */
+        char *event_name = NULL;
+        char *cmd = pop_next_command(&rule_list,
+                &event_name,       /* return event_name */
+                (dd ? &dd : NULL), /* match this dd... */
+                dump_dir_name,     /* ...or if NULL, this dirname */
+                pfx, pfx_len       /* for events with this prefix */
+        );
+        if (!cmd)
+            break;
+        free(cmd);
 
-        while (1) /* word loop */
+        if (event_name)
         {
-            char *end_word = skip_non_whitespace(p);
-            char *next_word = skip_whitespace(end_word);
-            *end_word = '\0';
-
-            /* If there is no '=' in this word... */
-            char *line_val = strchr(p, '=');
-            if (!line_val)
-                break; /* ...we found the start of a command */
-
-            /* Current word has VAR=VAL form. line_val => VAL */
-            *line_val++ = '\0';
-
-            /* Is it EVENT? */
-            if (strcmp(p, "EVENT") == 0)
+            /* Append "EVENT\n" - only if it is not there yet */
+            unsigned e_len = strlen(event_name);
+            char *p = result->buf;
+            while (p && *p)
             {
-                if (strncmp(line_val, pfx, pfx_len) != 0)
-                    goto next_line; /* prefix doesn't match */
-                /* (Ab)use line to save matching "\nEVENT_VAL\n" */
-                sprintf(line, "\n%s\n", line_val);
-            }
-            else
-            {
-                /* Get this name from dump dir */
-                if (!dd)
-                {
-                    /* Without dir name to match, we assume match for this expr */
-                    if (!dump_dir_name)
-                        goto next_word;
-                    dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
-                    if (!dd)
-                    {
-                        error = -1;
-                        free(line);
-                        free(next_line);
-                        goto stop; /* error (note: dd_opendir logged error msg) */
-                    }
-                }
-                char *real_val = dd_load_text_ext(dd, p, DD_FAIL_QUIETLY_ENOENT);
-                /* Does VAL match? */
-                if (strcmp(real_val, line_val) != 0)
-                {
-                    //VERB3 log("var '%s': '%.*s'!='%s', skipping line",
-                    //        p,
-                    //        (int)(strchrnul(real_val, '\n') - real_val), real_val,
-                    //        line_val);
-                    free(real_val);
-                    goto next_line; /* no */
-                }
-                free(real_val);
+                if (strncmp(p, event_name, e_len) == 0 && p[e_len] == '\n')
+                    goto skip; /* This event is already in the result */
+                p = strchr(p, '\n');
+                if (p)
+                    p++;
             }
-
- next_word:
-            /* Go to next word */
-            p = next_word;
-        } /* end of word loop */
-
-        if (line[0] == '\n' /* do we *have* saved matched "\nEVENT_VAL\n"? */
-         /* and does result->buf NOT yet have VAL? */
-         && strncmp(result->buf, line + 1, strlen(line + 1)) != 0
-         && !strstr(result->buf, line)
-        ) {
-            /* Add "EVENT_VAL\n" */
-            strbuf_append_str(result, line + 1);
+            strbuf_append_strf(result, "%s\n", event_name);
+ skip:
+            free(event_name);
         }
+    }
 
- next_line:
-        free(line);
-    } /* end of line loop */
-
- stop:
-    if (dump_dir_name != NULL)
-        dd_close(dd);
-    fclose(conffile);
-
-    return error;
-}
+    free_rule_list(rule_list);
 
-char *list_possible_events(struct dump_dir *dd, const char *dump_dir_name, const char *pfx)
-{
-    struct strbuf *result = strbuf_new();
-    int error = list_possible_events_helper(result, dd, dump_dir_name, pfx, CONF_DIR"/abrt_event.conf");
-    if (error)
-        strbuf_clear(result);
     return strbuf_free_nobuf(result);
 }