On Thu, 2011-08-04 at 12:12 +0200, Nikola Pajkovsky wrote:
+
+#include "abrtlib.h"
+#include "run_event.h"
+
+static int is_crash_a_dup(const char *dump_dir_name, void *param)
+{
+ struct cdump_state *state = (struct cdump_state *)param;
struct cdump_state is a local helper structure.
Its role is to make it possible to find state.crash_dump_dup_name
for the caller of run_event_on_dir_name().
Since now this code lives in a much smaller program,
I think you can just declare a few statics instead:
static char *uid;
static char *uuid;
static char *crash_dump_dup_name;
and use them instead of state->foo.
+ mw_result_t res = MW_ERROR;
...
+ res = MW_ERROR;
...
+ return res;
+}
Write-only variable :D
And btw, let's kill MW_FOO for good. Just use 0 for success and 1+ for
error, since here it's just the exit code of the program.
+ log("STATUS: %s", dump_dir_name);
--- a/src/daemon/abrtd.c
+++ b/src/daemon/abrtd.c
@@ -60,6 +60,7 @@ static GIOChannel *socket_channel = NULL;
static guint socket_channel_cb_id = 0;
static int socket_client_count = 0;
+static int timeout_sec = 60;
I don't think it's a good idea to limit even execution time, especially
by such a small constant. Uploading events can take many minutes.
-
-/* Inotify handler */
-
This comment should stay.
-struct logging_state {
- char *last_line;
-};
-
-static char *do_log_and_save_line(char *log_line, void *param)
-{
- struct logging_state *l_state = (struct logging_state *)param;
-
- VERB1 log("%s", log_line);
- update_client("%s", log_line);
- free(l_state->last_line);
- l_state->last_line = log_line;
- return NULL;
-}
static mw_result_t LoadDebugDump(const char *dump_dir_name,
problem_data_t **problem_data)
{
- mw_result_t res;
+ mw_result_t res = MW_ERROR;
Two spaces.
struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
if (!dd)
return MW_ERROR;
struct cdump_state state;
state.uid = dd_load_text(dd, FILENAME_UID);
state will be unused:
- state.uuid = NULL;
- state.crash_dump_dup_name = NULL;
- char *analyzer = dd_load_text(dd, FILENAME_ANALYZER);
dd_close(dd);
...
+ uid_t uid = xatoi_positive(state.uid);
The only use of state.uid is above and...
+
+ char *args[5];
+ args[0] = (char *) "abrt-handle-event";
+ args[1] = (char *) "-e";
+ args[2] = (char *) "post-create";
+ args[3] = (char *) dump_dir_name;
+ args[4] = NULL;
+
+ int pipeout[2];
+ int flags = EXECFLG_INPUT_NUL | EXECFLG_OUTPUT | EXECFLG_QUIET;
+ flags |= EXECFLG_ERR2OUT;
+ VERB1 flags &= ~EXECFLG_QUIET;
+
+ pid_t child = fork_execv_on_steroids(flags, args, pipeout,
+ /*env_vec:*/ NULL, /*dir:*/ NULL, uid);
...here, but flags have no EXECFLG_SETGUID and therefore
fork_execv_on_steroids() will simply ignore uid parameter.
Remove struct cdump_state state form this function.
+ int t = time(NULL); /* int is enough, no need to use time_t */
+ int endtime = t + timeout_sec;
+ struct strbuf *buf_out = strbuf_new();
+ while (1)
{
- /* No. Was there error on one of processing steps in run_event? */
- if (r != 0)
- goto ret; /* yes */
-
- /* Was uuid created after all? (In this case, is_crash_a_dup()
- * should have fetched it and created state.uuid)
- */
- if (!state.uuid)
+ int timeout = endtime - t;
+ if (timeout < 0)
{
- /* no */
- log("Dump directory '%s' has no UUID element",
dump_dir_name);
- goto ret;
+ kill(child, SIGKILL);
+ strbuf_append_strf(buf_out, "\nTimeout exceeded: %u seconds, killing
%s\n", timeout_sec, args[0]);
+ break;
}
+
+ /* We don't check poll result - checking read result is enough */
+ struct pollfd pfd;
+ pfd.fd = pipeout[0];
+ pfd.events = POLLIN;
+ poll(&pfd, 1, timeout * 1000);
+
+ char buff[1024];
+ int r = read(pipeout[0], buff, sizeof(buff) - 1);
+ if (r <= 0)
+ break;
+ buff[r] = '\0';
+ strbuf_append_str(buf_out, buff);
+ t = time(NULL);
+ }
+ close(pipeout[0]);
I think in this case there should not be any timeout.
+ if (buf_out->buf[buf_out->len - 1] == '\n')
+ buf_out->buf[buf_out->len - 1] = '\0';
What if buf_out->len == 0?
+ char *last_line = strrchr(buf_out->buf, '\n');
+ if (!last_line)
+ {
+ if (prefixcmp("STATUS: ", buf_out->buf))
+ dump_dir_name = buf_out->buf + sizeof("STATUS: ");
}
else
{
- dump_dir_name = state.crash_dump_dup_name;
+ if (prefixcmp("STATUS: ", buf_out->buf))
^^^^^^^^^^^^^ BUG?
+ dump_dir_name = last_line + sizeof("STATUS:
");
}
Can you do this instead of big if()? -
+ char *last_line = strrchr(buf_out->buf, '\n');
+ if (!last_line)
+ last_line = buf_out->buf;
+ if (prefixcmp....
+typedef enum {
+ MW_OK, /**< No error.*/
+ MW_OCCURRED, /**< No error, but thus dump is a dup.*/
+ MW_ERROR, /**< Common error.*/
+} mw_result_t;
+
+/* We need to share some data between LoadDebugDump and is_crash_a_dup: */
+struct cdump_state {
+ char *uid;
+ char *uuid;
+ char *crash_dump_dup_name;
+};
Remove struct cdump_state from here.
Logic note: only post-create event needs to do this dance around uuid
and duplication detection, but abrt-handle-event, as implemented,
seems to do it regardless of event name.
I think we should either check event_name == "post-create", or
have a switch which enables "the dance" (the
run_state->post_run_callback = is_crash_a_dup
thing).
--
vda