On Tue, 2010-05-25 at 16:20 +0200, Karel Klic wrote:
Hi,
All patches attached, but only 0002-* is updated. Please see comments below.
I see that you implemented timing out connections! Nice!
+ char *newpath = xstrndup(path, strlen(path) - strlen(".new")); + if (rename(path, newpath) != 0) + strcpy(path, newpath); ... + if (maxCrashReportsSize > 0) + { + check_free_space(maxCrashReportsSize); + trim_debug_dumps(maxCrashReportsSize, path); + }
I think this is a originally my bug, should be ==, not !=: if (rename(path, newpath) == 0) strcpy(path, newpath); because the goal of strcpy is to set path to the name of directory which must not be deleted by trim_debug_dumps(). If rename() == 0 [that is, it succeeded], now the name is "newpath", and we must use it, not "path".
+/* Callback called by glib main loop when a client newly opens ABRT's socket. */ +static gboolean server_socket_cb(GIOChannel *source, + GIOCondition condition, + gpointer data) +{ + if (condition & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) + { + error_msg("dumpsocket: Server socket error"); + return TRUE; + }
Why bother checking this? accept() will deal with errors (by setting errno and returning -1).
+ /* Check the limit for number of simultaneously attached clients. */ + if (client_count >= MAX_CLIENT_COUNT) + { + error_msg("dumpsocket: Too many clients, refusing connection."); + return TRUE; + }
This will leave descriptor in "ready" state, and it will immediately be picked by next loop iteration. The symptom is that when MAX_CLIENT_COUNT is reached, abrtd will start eating 100% CPU. You need to remove the fd from main loop's set of descriptors to watch, and re-add it whenever you see --client_count == MAX_CLIENT_COUNT-1 in client_free().
+ struct sockaddr_un remote; + socklen_t len = sizeof(remote); + int socket = accept(g_io_channel_unix_get_fd(source), (struct sockaddr*)&remote, &len); + if (socket == -1) + { + error_msg("dumpsocket: Server can not accept client"); + return TRUE; + }
(1) Maybe perror_msg? errno value is useful to know in this case... (2) On siccess, do you need to set this new socket "close on exec" too?
+ int socketfd = socket(AF_UNIX, SOCK_STREAM, 0); + if (socketfd == -1) + perror_msg_and_die("dumpsocket: Can't create AF_UNIX socket"); + + /* Set the FD_CLOEXEC flag to the socket. */ + fcntl(socketfd, F_SETFD, fcntl(socketfd, F_GETFD) | FD_CLOEXEC); + + memset(&local, 0, sizeof(local)); + local.sun_family = AF_UNIX; + strcpy(local.sun_path, SOCKET_FILE); + if (bind(socketfd, (struct sockaddr*)&local, sizeof(local)) == -1) + perror_msg_and_die("dumpsocket: Can't bind AF_UNIX socket to '%s'", SOCKET_FILE); + + if (listen(socketfd, MAX_CLIENT_COUNT) != 0) + perror_msg_and_die("dumpsocket: Can't listen on AF_UNIX socket");
We have xfunctions to make it much more compact: ... int socketfd = xsocket(AF_UNIX, SOCK_STREAM, 0); close_on_exec_on(socketfd); memset(&local, 0, sizeof(local)); local.sun_family = AF_UNIX; strcpy(local.sun_path, SOCKET_FILE); xbind(socketfd, (struct sockaddr*)&local, sizeof(local)); xlisten(socketfd, MAX_CLIENT_COUNT);