Thank you very much indeed! On Wednesday 14 of November 2012 19:00:40 Martin Milata wrote: > I run-tested the branch and it seems to be working well. I also reviewed > the code but due to my lack of experience with gtk/dbus/libnotify/NM I > probably missed the major flaws if there are any:) For smaller issues, > see my comments inline. > > Side notes: > - How can I trigger unknown problem without creating bogus problem > report for some package? (Can we include a program in will_crash that > will crash with stack random enough that the clustering algorithm > always fails?:))
Or we can hack the server to always return unknown response for some executable shipped in will_crash package. > - On my system, the notification shows after reporter-mailx has > finished (or so it seems), which takes some time (maybe my MTA is > misconfigured?). Can we reorder the events so that the applet shows > immediately? See https://fedorahosted.org/abrt/ticket/871 > > On Mon, Nov 12, 2012 at 19:14:32 +0100, Jakub Filak wrote: > > diff --git a/src/applet/applet.c b/src/applet/applet.c > > index c34e4c6..68180fe 100644 > > --- a/src/applet/applet.c > > +++ b/src/applet/applet.c > > > > +static GList *g_deffered_crash_queue; > > +static guint g_deffered_timeout = 0; > > Deffered is actually spelled deferred. > > > * in $XDG_CACHE_HOME/abrt/applet_dirlist. In any case, applet_dirlist > > * is updated with updated list. > > */ > > > > -static GList *new_dir_exists(void) > > +static void new_dir_exists(GList **new_dirs) > > > > { > > Please update the comment of new_dir_exists, it does not reflect what > the function does (i.e. does not return anything). Also update the > comments at the function call sites. will do > > > static void fork_exec_gui(void) > > { > > > > - pid_t pid = vfork(); > > + pid_t pid = fork(); > > > > if (pid < 0) > > > > - perror_msg("vfork"); > > + perror_msg("fork"); > > > > if (pid == 0) > > { > > > > /* child */ > > > > - signal(SIGCHLD, SIG_DFL); /* undo SIG_IGN in abrt-applet */ > > -//TODO: pass s_dirs[] as DIR param(s) to abrt-gui > > + /* double fork to avoid GUI zombies */ > > + pid_t grand_child = vfork(); > > + if (grand_child != 0) > > + { > > + if (grand_child < 0) perror_msg("vfork"); > > + exit(0); > > + } > > + > > + /* grand child */ > > + //TODO: pass s_dirs[] as DIR param(s) to abrt-gui > > > > execl(BIN_DIR"/abrt-gui", "abrt-gui", (char*) NULL); > > /* Did not find abrt-gui in installation directory. Oh well */ > > /* Trying to find it in PATH */ > > execlp("abrt-gui", "abrt-gui", (char*) NULL); > > perror_msg_and_die("Can't execute '%s'", "abrt-gui"); > > > > } > > > > + > > + safe_waitpid(pid, /* status */ NULL, /* options */ 0); > > + > > If I understand vfork() correctly, doing anything else than exec*/_exit > (e.g. perror_msg_and_die) results in undefined behaviour. We can either > use fork() here or simply _exit on error. I'll replace vfork() by fork() > > > + notify_notification_add_action(notification, > > A_KNOWN_OPEN_GUI, _("Open"), > ... > > > + notify_notification_add_action(notification, > > A_KNOWN_OPEN_BROWSER, _("Show"), > I think "Open" and "Show" sound a bit similar, but I can't think of > anythig better:( Can we use two+ words? Yes, we can. But the label must be really short otherwise label's text will be shortened by "...". > > > + /* Read streamed data and split lines */ > > + for (;;) > > + { > > + char buf[250]; /* usually we get one line, no need to have big > > buf */ + errno = 0; > > + gsize r = 0; > > + g_io_channel_read_chars(gio, buf, sizeof(buf) - 1, &r, NULL); > > + if (r <= 0) > > + break; > > + buf[r] = '\0'; > > + > > + /* split lines in the current buffer */ > > + char *raw = buf; > > + char *newline; > > + while ((newline = strchr(raw, '\n')) != NULL) > > + { > > + *newline = '\0'; > > + strbuf_append_str(state->cmd_output, raw); > > + char *msg = state->cmd_output->buf; > > > > - notify_notification_add_action(notification, "REPORT", _("Report"), > > - > > NOTIFY_ACTION_CALLBACK(action_report), > > - pi, NULL); > > + VERB3 log("%s", msg); > > + pi->known |= (prefixcmp(msg, "THANKYOU") == 0); > > > > - notify_notification_update(notification, _("A Problem has Occurred"), > > buf, NULL); - free(buf); > > + strbuf_clear(state->cmd_output); > > + /* jump to next line */ > > + raw = newline + 1; > > + } > > > > - GError *err = NULL; > > - notify_notification_show(notification, &err); > > - if (err != NULL) > > + /* beginning of next line. the line continues by next read */ > > + strbuf_append_str(state->cmd_output, raw); > > + } > > + > > + if (errno == EAGAIN) > > Does g_io_channel_read_chars set the errno? I can't find any mention of > it in the documentation. Good point! I've blindly reused an old code. The correct solutions is to check a return value of g_io_channel_read_chars() call to G_IO_STATUS_AGAIN constant instead of checking of errno. > > Martin
