[syslog-ng] Unix-stream destination as listen socket
Balazs Scheidler
bazsi at balabit.hu
Thu Oct 20 11:02:33 CEST 2005
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
More information about the syslog-ng
mailing list