Hi, Thanks for your patches Gergely, but I would rather see the constructor to be passed a LogTemplate instance instead of adding an error path to the constructor. This is true to all other patches in the series. I integrating all your patches from merge-queue but will skip those affected by this comment. Cheers, On h, 2013-06-10 at 16:54 +0200, Gergely Nagy wrote:

When there is a syntax error in the filename template (eg, "/var/log/foo-$(+ 0"), error out at config load time. This is done by adding an error output variable to affile_dd_new_instance() and both affile_dd_new() and afpipe_dd_new(), then using that to store the result of log_template_compile(). If that function fails, we bail out early, and use CHECK_ERROR() in the grammar to notify the user of the issue.

Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- modules/affile/affile-dest.c | 23 +++++++++++++++-------- modules/affile/affile-dest.h | 6 +++--- modules/affile/affile-grammar.ym | 12 +++++++++--- 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/modules/affile/affile-dest.c b/modules/affile/affile-dest.c index c8fa9c1..c9447ce 100644 --- a/modules/affile/affile-dest.c +++ b/modules/affile/affile-dest.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2002-2012 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2002-2013 BalaBit IT Ltd, Budapest, Hungary * Copyright (c) 1998-2012 Balázs Scheidler * * This program is free software; you can redistribute it and/or modify it @@ -703,17 +703,24 @@ affile_dd_free(LogPipe *s) }

static AFFileDestDriver * -affile_dd_new_instance(gchar *filename) +affile_dd_new_instance(gchar *filename, GError **error) { AFFileDestDriver *self = g_new0(AFFileDestDriver, 1); + LogTemplate *filename_template; + + filename_template = log_template_new(configuration, NULL); + if (!log_template_compile(filename_template, filename, error)) + { + log_template_unref(filename_template); + return NULL; + }

log_dest_driver_init_instance(&self->super); self->super.super.super.init = affile_dd_init; self->super.super.super.deinit = affile_dd_deinit; self->super.super.super.queue = affile_dd_queue; self->super.super.super.free_fn = affile_dd_free; - self->filename_template = log_template_new(configuration, NULL); - log_template_compile(self->filename_template, filename, NULL); + self->filename_template = filename_template; log_writer_options_defaults(&self->writer_options); file_perm_options_defaults(&self->file_perm_options); self->writer_options.mark_mode = MM_NONE; @@ -728,15 +735,15 @@ affile_dd_new_instance(gchar *filename) }

LogDriver * -affile_dd_new(gchar *filename) +affile_dd_new(gchar *filename, GError **error) { - return &affile_dd_new_instance(filename)->super.super; + return &affile_dd_new_instance(filename, error)->super.super; }

LogDriver * -afpipe_dd_new(gchar *filename) +afpipe_dd_new(gchar *filename, GError **error) { - AFFileDestDriver *self = affile_dd_new_instance(filename); + AFFileDestDriver *self = affile_dd_new_instance(filename, error); self->is_pipe = TRUE; return &self->super.super; } diff --git a/modules/affile/affile-dest.h b/modules/affile/affile-dest.h index 3f4f21b..dcee654 100644 --- a/modules/affile/affile-dest.h +++ b/modules/affile/affile-dest.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2002-2012 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2002-2013 BalaBit IT Ltd, Budapest, Hungary * Copyright (c) 1998-2012 Balázs Scheidler * * This program is free software; you can redistribute it and/or modify it @@ -53,8 +53,8 @@ typedef struct _AFFileDestDriver gint time_reap; } AFFileDestDriver;

-LogDriver *affile_dd_new(gchar *filename); -LogDriver *afpipe_dd_new(gchar *filename); +LogDriver *affile_dd_new(gchar *filename, GError **error); +LogDriver *afpipe_dd_new(gchar *filename, GError **error);

void affile_dd_set_create_dirs(LogDriver *s, gboolean create_dirs); void affile_dd_set_fsync(LogDriver *s, gboolean enable); diff --git a/modules/affile/affile-grammar.ym b/modules/affile/affile-grammar.ym index abecee4..0d2f099 100644 --- a/modules/affile/affile-grammar.ym +++ b/modules/affile/affile-grammar.ym @@ -1,5 +1,5 @@ /* - * Copyright (c) 2002-2012 BalaBit IT Ltd, Budapest, Hungary + * Copyright (c) 2002-2013 BalaBit IT Ltd, Budapest, Hungary * Copyright (c) 1998-2012 Balázs Scheidler * * This program is free software; you can redistribute it and/or modify it @@ -144,7 +144,10 @@ dest_affile dest_affile_params : string { - last_driver = *instance = affile_dd_new($1); + GError *error; + + last_driver = *instance = affile_dd_new($1, &error); + CHECK_ERROR(last_driver, @1, "Error compiling template; %s", error->message); free($1); last_writer_options = &((AFFileDestDriver *) last_driver)->writer_options; last_file_perm_options = &((AFFileDestDriver *) last_driver)->file_perm_options; @@ -171,7 +174,10 @@ dest_affile_option dest_afpipe_params : string { - last_driver = *instance = afpipe_dd_new($1); + GError *error; + + last_driver = *instance = afpipe_dd_new($1, &error); + CHECK_ERROR(last_driver, @1, "Error compiling template; %s", error->message); free($1); last_writer_options = &((AFFileDestDriver *) last_driver)->writer_options; last_file_perm_options = &((AFFileDestDriver *) last_driver)->file_perm_options;