Hi,
All patches attached, but only 0002-* is updated. Please see comments below.
Karel
On 05/17/2010 02:56 PM, Denys Vlasenko wrote:
The number of accepted sockets was limited by the listen() call. I made the limit more explicit (defined as MAX_CLIENT_COUNT).
listen() only limits how many new connections will be kept buffered by kernel, waiting for your accept() call. If you don't limit the number of accept()s you do, then number of connections will not be limited.
Correct. Fixed.
Not sure what is the best way how to implement the timeout.
Is there any (glib?) trick that can do the timeout for me?
Here is how I would do it:
- track the last client write time in struct client
- on client connection a timer is enabled (wakeup every N seconds)
- on wakeup the timer checks the last write time for every opened
socket and if it is more than N seconds in past, the socket is closed 4) if the number of opened sockets is 0, disable the timer
Does someone have better idea?
Sounds ok.
Implemented in slightly different way. 1) track the last client write time in struct client 2) on client connection a client-specific timer is enabled (wakeup every N seconds) 3) on wakeup the timer checks the last write time for its socket and if it is more than N seconds in past, the socket is closed 4) when closing the socket, the timer is removed
I rewrote the code to use static array and to read incoming data in chunks.
static gchar buf[INPUT_BUFFER_SIZE];
Why "static"? Since it's 1k, you can remove "static" and keep it on stack. This way, after use this 1k block won't take up RAM...
I felt that adding/removing 1 kB on/from stack every time is worse than using heap. Removed `static`.
Review of new patch:
- char *path = xasprintf(DEBUG_DUMPS_DIR"/%s-%ld-%u.new",
client->basename,
(long)time(NULL),
client->pid);
You forgot to free it.
Fixed.
strlen("PID="), strlen(".new") gcc is still stupid and emits strlen() calls for constant strings. sizeof("CONST")-1 is the same as strlen("CONST"), but evaluates at compile time.
I prefer to keep it as strlen(), because of code readability.
When I first read about sizeof("text"), I thought it might be a bug as it was supposed to return the same size as sizeof(pointer) :)
From nitpicking department:
+#define HANDLE_INCOMING_STRING(__tag, __field, __max_len, __printable, __allow_slashes) \
- char *__field = try_to_get_string(message, __tag, __max_len,
__printable, __allow_slashes); \
- if (__field) \
- { \
free(client->__field); \
client->__field = __field; \
return; \
- }
Names with leading __underscores are reserved for compiler and libc. Use plain names without underscores.
True, changed.
- Module for a ABRT exception handling hook
"the" ABRT exception handling hook. We don't have many ABRT exception handling hooks, "a" implies we do.
Fixed.
- Caller is responsible to call free() to the returned
- pointer.
"to call free() ON the returned pointer"
Fixed.
messsage -- three 's'
Fixed.