On Wed, 2005-10-19 at 14:33 -0400, Peter Nahas wrote:
Per Balazs' suggestion, I have implemented an AFSocketCallHomeDestDriver "call home" destination driver and a unix_call_home_stream destination to implement the feature and have attached a patch against syslog-ng-1.9.6 along with this email. My implementation currently only supports one incoming connection and currently only adds a unix stream version, however it would be easy to enhance the implementation to support TCP and/or support more connections. If the patch is not accepted into the repository, I would like to know what requirements must be met to do so.
First of all, thanks for your contribution. I have some minor technical comments, please find those inline. The change as a whole is very nicely implemented. That said the work you have done is already significant enough to be copyrightable and I have to talk to our lawyers first. BalaBit would like to keep the possibility to relicense syslog-ng under different licenses and integrating this patch would prevent that. I'll come back to you once I have an answer. Now my comments:
plain text document attachment (patch) diff -urP 1.9.6/afsocket.c mine/afsocket.c --- 1.9.6/afsocket.c 2005-10-19 14:10:46.000000000 -0400 +++ mine/afsocket.c 2005-10-19 14:17:19.000000000 -0400 @@ -764,3 +764,150 @@ self->super.super.notify = afsocket_dd_notify; self->flags = flags; } + +/******************************************************************************* + * + * PCN - added "call home driver" + * The purpose of the CallHomeDestinationDriver is to provide a system by which + * a destination may connect to syslog-ng to received data rather than the + * other way around. + * + ******************************************************************************/
Please do not add shouting comments like this, a simple elegant /* * */ comment block should suffice. and please do not add your name to one-line changes, it bloats code and has no associated information. This information belongs to the version control system and/or the AUTHORS file.
+gboolean +afsocket_chdd_process_connection(AFSocketCallHomeDestDriver *self, GSockAddr *peer_addr, gint fd) +{ + if(self->connectedFd > 0) + { + return FALSE; + } + + self->connectedFd = fd; + log_writer_reopen(self->writer, fd_write_new(self->connectedFd)); + if (!log_pipe_init(self->writer, NULL, NULL)) + return FALSE; + return TRUE; +}
In syslog-ng I use GNU coding style with GLib naming style. connectedFd should be called connected_fd.
+ +static gboolean +afsocket_chdd_accept(gpointer s) +{ + AFSocketCallHomeDestDriver *self = (AFSocketCallHomeDestDriver *) s; + GSockAddr *peer_addr; + gchar buf1[256], buf2[256]; + gint new_fd; + gboolean res; + + if (g_accept(self->listenFd, &new_fd, &peer_addr) != G_IO_STATUS_NORMAL) + { + msg_error("Error call home accepting new connection", + evt_tag_errno(EVT_TAG_OSERROR, errno), + NULL); + return TRUE; + } + g_fd_set_nonblock(new_fd, TRUE); + + msg_verbose("Call home connection accepted", + evt_tag_str("from", g_sockaddr_format(peer_addr, buf1, sizeof(buf1))), + evt_tag_str("to", g_sockaddr_format(self->bind_addr, buf2, sizeof(buf2))), + NULL); + + res = afsocket_chdd_process_connection(self, peer_addr, new_fd); + g_sockaddr_unref(peer_addr); + return res; + +}
similarly, listenFd should be named listen_fd
+ +static void +afsocket_chdd_notify(LogPipe *s, LogPipe *sender, gint notify_code, gpointer user_data) +{ + AFSocketCallHomeDestDriver *self = (AFSocketCallHomeDestDriver *) s; + switch (notify_code) + { + case NC_CLOSE: + case NC_WRITE_ERROR: + self->connectedFd = -1; + break; + } +}
possible fd-leak?
diff -urP 1.9.6/cfg-grammar.y mine/cfg-grammar.y --- 1.9.6/cfg-grammar.y 2005-10-19 14:10:46.000000000 -0400 +++ mine/cfg-grammar.y 2005-10-19 14:13:33.000000000 -0400 @@ -48,7 +48,7 @@ %token KW_SOURCE KW_DESTINATION KW_LOG KW_OPTIONS KW_FILTER
/* source & destination items */ -%token KW_INTERNAL KW_FILE KW_PIPE KW_UNIX_STREAM KW_UNIX_DGRAM KW_UDP +%token KW_INTERNAL KW_FILE KW_PIPE KW_UNIX_STREAM KW_UNIX_CHSTREAM KW_UNIX_DGRAM KW_UDP %token KW_TCP KW_USER %token KW_DOOR KW_SUN_STREAMS KW_PROGRAM
@@ -142,6 +142,7 @@ %type <ptr> dest_afsocket %type <ptr> dest_afunix_dgram_params %type <ptr> dest_afunix_stream_params +%type <ptr> dest_afunix_ch_stream_params /* PCN added for call home driver */
again, the comment does not belong here.
@@ -525,6 +527,16 @@ dest_writer_options { $$ = last_driver; } ;
+// PCN added for call home driver
again
+dest_afunix_ch_stream_params + : string + { + last_driver = afunix_chdd_new($1, AFSOCKET_STREAM); + free($1); + last_writer_options = &((AFSocketCallHomeDestDriver *) last_driver)->writer_options; + } + dest_writer_options { $$ = last_driver; } + ;
dest_afinet_udp_params : string diff -urP 1.9.6/cfg-lex.l mine/cfg-lex.l --- 1.9.6/cfg-lex.l 2005-10-19 14:10:46.000000000 -0400 +++ mine/cfg-lex.l 2005-10-19 14:15:27.000000000 -0400 @@ -19,7 +19,7 @@ * * Inspired by nsyslog, originally written by Darren Reed. * - * $Id: cfg-lex.l,v 1.8 2003/01/22 11:11:18 bazsi Exp $ + * $Id$ * ***************************************************************************/ %{ @@ -52,6 +52,7 @@ { "internal", KW_INTERNAL }, { "unix_dgram", KW_UNIX_DGRAM }, { "unix_stream", KW_UNIX_STREAM }, + { "unix_call_home_stream", KW_UNIX_CHSTREAM },
I'd say the name is a little bit too long for the configuration file. It is one matter that we have long names in the code, as that makes the code more readable, but I think we should not force the user to use it this way in the configuration file. I don't see any better name than "unix-ch-stream" though. Any good ideas? -- Bazsi