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.