Pushed to git (with Denys's agreement).
On 06/08/2010 03:57 PM, Karel Klic wrote:
Hi,
as usual, only patch no. 2 is updated. Please see the comments below.
On 05/26/2010 04:59 PM, Denys Vlasenko wrote:
- 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".
Yes, fixed.
+/* 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).
Yes, removed also from g_io_add_watch.
- /* 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().
So I added: /* Check the limit for number of simultaneously attached clients. */ if (client_count >= MAX_CLIENT_COUNT) { error_msg("dumpsocket: Too many clients, refusing connection."); g_source_remove(channel_cb_id); channel_cb_id = 0; return TRUE; }
And to client_free(): --client_count; if (!channel_cb_id) { channel_cb_id = g_io_add_watch(channel, (GIOCondition)(G_IO_IN | G_IO_PRI), (GIOFunc)server_socket_cb, NULL); if (!channel_cb_id) perror_msg_and_die("dumpsocket: Can't add socket watch"); }
This should also be valid in all cases.
Here is a snippet that tests it (abrtd -dvvv running): #!/usr/bin/python import socket slist = [] for i in range(0, 50): s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) s.connect("/var/run/abrt.socket") s.sendall("PID=%s\0" % 100) slist.append(s)
- 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...
Yes, added.
(2) On siccess, do you need to set this new socket "close on exec" too?
Yes.
- 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);
Good. I changed the code to use them.
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher