On 08/21/2013 12:22 PM, Jakub Filak wrote:
+struct abrt_inotify_watch *
+abrt_inotify_watch_init(const char *path, int inotify_flags, abrt_inotify_watch_handler
handler, void *user_data)
+{
+ struct abrt_inotify_watch *aiw = malloc(sizeof(*aiw));
Let's use xmalloc here.
+void
+abrt_inotify_watch_destroy(struct abrt_inotify_watch *watch)
+{
+ if (!watch)
+ return;
+
+ inotify_rm_watch(watch->inotify_fd, watch->inotify_wd);
+ g_source_remove(watch->channel_inotify_source_id);
+
+ GError *error = NULL;
+ g_io_channel_shutdown(watch->channel_inotify, FALSE, &error);
+ if (error)
+ {
+ VERB1 log("Can't shutdown inotify gio channel: '%s'",
error ? error->message : "");
We know that error is not NULL (we checked it in if()).
+#define STRINGIZE_DETAIL(str) #str
+#define STRINGIZE(str) STRINGIZE_DETAIL(str)
+
+#define DEFAULT_COUNT_OF_WORKERS 10
+#define DEFAULT_CACHE_MIB_SIZE 4
+#define APPROX_BYTES_PER_ARCHIVE_NAME FILENAME_MAX
APPROX_BYTES_PER_ARCHIVE_NAME is unused.
In general I think the feature of having a list
of deferred archive names to process is not needed.
The purpose of limiting max number of workers is to
prevent either an attack (someone creates tons of
archives) or a bug in abrt-handle-upload
where it never exits.
In both such scenarios the list
of deferred archive names doesn't help - it overflows too.
The only thing this list achieves is ability to raise
the number of processed archives to a largish number
without using lots of memory: remembering 1000 names
is cheaper than spawning 1000 abrt-handle-upload's.
But, do we actually expect to support such large
numbers of simultaneous archives? I don't think so,
supporting 10-30 simultaneously appearing archives
is probably enough. Mechanism of DEFAULT_COUNT_OF_WORKERS
is enough for such numbers.
I propose dropping DEFAULT_CACHE_MIB_SIZE, the queue thing,
and -c CACHESIZE option.
If -w NUM workers are exceeded, just ignore the new archive.
+struct process
+{
+ GMainLoop *main_loop;
+ const char *upload_directory;
+ unsigned children;
+ unsigned max_children;
+ struct queue queue;
+};
You have just one "struct process" object.
I would just use static variables...
+static void
+decrement_child_count(struct process *proc)
+{
+ --proc->children;
+
+ char *name = queue_pop(&proc->queue);
+ if (!name)
+ {
+ VERB3 log("Deferred queue is empty. Running workers: %d",
proc->children);
+ return;
+ }
+
+ run_abrt_handle_upload(proc, name);
+ free(name);
+}
+
+static void
+handle_signal(int signo)
+{
+ int save_errno = errno;
+ uint8_t sig_caught = signo;
+ if (write(g_signal_pipe[1], &sig_caught, 1))
+ /* we ignore result, if () shuts up stupid compiler */;
+ errno = save_errno;
+}
+
+static gboolean
+handle_sigchld_pipe(GIOChannel *gio, GIOCondition condition, gpointer user_data)
+{
+ struct process *proc = (struct process *)user_data;
+ uint8_t signals[DEFAULT_COUNT_OF_WORKERS];
+ gsize len = 0;
+
+ for (;;)
+ {
+ GError *error = NULL;
+ GIOStatus stat = g_io_channel_read_chars(gio, (void *)signals, sizeof(signals),
&len, NULL);
+ if (stat == G_IO_STATUS_ERROR)
+ {
+ error_msg_and_die(_("Can't read from gio channel:
'%s'"), error ? error->message : "");
+ }
+ if (stat == G_IO_STATUS_AGAIN)
+ { /* We got all buffered data, but fd is still open. Done for now */
+ return TRUE; /* "glib, please don't remove this event (yet)"
*/
+ }
+ if (stat == G_IO_STATUS_EOF)
+ break;
+
+ /* G_IO_STATUS_NORMAL */
+ for (unsigned signo = 0; signo < len; ++signo)
+ {
+ /* we did receive a signal */
+ VERB3 log("Got signal %d through signal pipe", signals[signo]);
+ if (signals[signo]!= SIGCHLD)
+ {
+ process_quit(proc);
+ return FALSE; /* remove this event */
+ }
+ else
+ {
+ while (safe_waitpid(-1, NULL, WNOHANG) > 0)
+ {
+ decrement_child_count(proc);
+ }
+ }
+ }
+ }
+
+ return TRUE; /* "please don't remove this event" */
+}
Signal pipe is needed if you can't do your job in signal handler
(because of restrictive environment there).
Here, if the queue thing is dropped, it seems to be possible
to do the necessary operations entirely in signal handler -
with a bit of cleverness with process counting.
What we basically need in signal handler is to wait for
children and decrement children count:
static void
handle_signal(int signo)
{
int save_errno = errno;
while (safe_waitpid(-1, NULL, WNOHANG) > 0)
{
child_count--;
}
errno = save_errno;
}
The problem is, child_count-- is not atomic, it can race with child_count++
elsewhere. (On x86, it _is_ atomic. On RISC, it may not be).
The trick here is to have two counters:
static unsigned children_created;
static unsigned children_died;
, do "children_died++" in signal handler, and NEVER touch children_died
anywhere else.
Then, in other place(s) you'll need to do this:
unsigned children_running = children_created - children_died;
if (children_running < max_children) {
...fork/exec...;
children_created++;
}
Note that it is safe wrt children_{created,died}++ overflowing
from 0xfffffffffffffff to 0. Subtraction will still give correct data.
Since SIGTERM and SIGINT don't actually do any special processing,
we can simply not install a handler for them.
Thus, signal pipe thingy can be avoided.
+static void
+handle_inotify_cb(struct abrt_inotify_watch *watch, struct inotify_event *event, void
*user_data)
+{
+ /* Was the (presumable newly created) file closed in upload dir,
+ * or a file moved to upload dir? */
+ if (event->name && !(event->mask & IN_ISDIR) &&
(event->mask & (IN_CLOSE_WRITE | IN_MOVED_TO)))
+ {
+ const char *ext = strrchr(event->name, '.');
+ if (ext && strcmp(ext + 1, "working") == 0)
+ return;
+
+ handle_new_path((struct process *)user_data, xstrdup(event->name));
+ }
+}
+
+static void
+daemonize()
+{
+ /* forking to background */
+ fflush(NULL); /* paranoia */
+ pid_t pid = fork();
+ if (pid < 0)
+ perror_msg_and_die("fork");
+ if (pid > 0)
+ exit(0);
+
+ /* Child (daemon) continues */
+ if (setsid() < 0)
+ perror_msg_and_die("setsid");
+
+ /* Change the current working directory */
+ if ((chdir("/")) < 0)
+ perror_msg_and_die("chdir(/)");
We have xchdir() which does the dying :)
+ /* Close out the standard file descriptors */
+ close(STDIN_FILENO);
+ close(STDOUT_FILENO);
+ close(STDERR_FILENO);
Reopening to "/dev/null" is considered to be a better practice.
--
vda