--- Begin Message ---
- Subject: Re: [netfilter-core] Patch for ULOGD
- From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
- Date: Thu, 6 Sep 2012 20:24:54 +0200
- Cc: coreteam@xxxxxxxxxxxxx
- In-reply-to: <5047576C.9020006@anduras.de>
- User-agent: Mutt/1.5.20 (2009-06-14)
Hi,
On Wed, Sep 05, 2012 at 03:45:16PM +0200, Gerfried Essler wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello,
>
> I wrote a patch for ULOGD for creating logfiles in a syslog style (with
> strftime macros),
> for example: "/var/log/ulogd/%Y/%m/%d/ulogd_syslogemu.log" to organize
> the logfiles
> in a Year/month/day directory structure.
>
> Compile ULOG with the --enable-logname-expansion flag to use it.
>
> If you have any questions please send an e-mail to me or to Sven Anders
> <anders@xxxxxxxxxx>
Please, send this patch to netfilter-devel mailing list.
A couple of quick comments:
> with kind regards,
> Gerfried Essler
> <essler@xxxxxxxxxx>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJQR1dsAAoJEC/R0e/1eoMpIf0H/jJZ60BKNTsPzN0tNH5T9XE3
> MjOcJrz/VpprLStP/g+fMkZHQOe1OI5kJ6avWXCiDGiRDdniI+Ju7Nm9iXhY05jp
> HLW+BAKj36PLu4qSpWgYrZbIsb8Zsb9SlBGK4PS7kA63YVSIlPy7dvrZbU3NbWCT
> meEkeKZa5RWKx18DP/x/4LYr1tQueXHJWFAnDZgnwEmXq0yNT4x0H3TCoVReysK8
> oMAzJVfRh7j5WBXG6qrfRf0kye1nfk4IZADtC5QKX9HbiKvl1krYV7+tZLtccE7/
> MPPKslH+Uc7lKBWbYstJfRSt59rrE86ZoxEd8IGYOgeQNGgtzoVhEz5NVYugBNY=
> =H1PU
> -----END PGP SIGNATURE-----
>
> --- ulogd-2.0.0.orig/configure.ac 2012-06-17 13:01:58.000000000 +0200
> +++ ulogd-2.0.0/configure.ac 2012-08-29 13:11:01.000000000 +0200
> @@ -2,7 +2,7 @@
> AC_PREREQ([2.50])
> AC_INIT([ulogd], [2.0.0])
> AC_CONFIG_AUX_DIR([build-aux])
> -AM_INIT_AUTOMAKE([-Wall foreign tar-pax no-dist-gzip dist-bzip2 1.10b])
> +AM_INIT_AUTOMAKE([-Wall foreign tar-pax no-dist-gzip dist-bzip2 1.10])
> AC_CONFIG_HEADER([config.h])
> AC_CONFIG_MACRO_DIR([m4])
>
> @@ -60,7 +60,13 @@
> CT_CHECK_DBI()
> AM_CONDITIONAL(HAVE_DBI, test "x$DBI_LIB" != "x")
>
> -
> +dnl
> +dnl test for logname expansion support
> +dnl
> +AC_ARG_ENABLE(logname-expansion,
> +[ --enable-logname-expansion enables logname expansion],[
> +CFLAGS="${CFLAGS} -DLOGNAME_EXPANSION"
> +])
Please, no conditional compilation options. This has to be there by
default and make sure this new parameters are optional and backward
compatibility is not broken.
> dnl AC_SUBST(DATABASE_DIR)
> dnl AC_SUBST(DATABASE_LIB)
> --- ulogd-2.0.0.orig/ulogd.conf.in 2012-05-18 02:49:10.000000000 +0200
> +++ ulogd-2.0.0/ulogd.conf.in 2012-09-05 15:06:42.000000000 +0200
> @@ -7,6 +7,7 @@
> # GLOBAL OPTIONS
> ######################################################################
>
> +nlgroup=1
>
> # logfile for status messages
> logfile="/var/log/ulogd.log"
> @@ -171,6 +172,18 @@
> file="/var/log/ulogd_syslogemu.log"
> sync=1
>
> +# The --enable-logname-expansion flag must be enabled to use these options
> +[emu2]
> +file="/var/log/ulogd/%Y/%m/%d/ulogd_syslogemu.log"
> +create_dirs=
> +uid=
> +gid=
> +dir_uid=
> +dir_gid=
> +mode=
> +dir_mode=
> +syslogsync=1
> +
> [op1]
> file="/var/log/ulogd_oprint.log"
> sync=1
> --- ulogd-2.0.0.orig/output/ulogd_output_LOGEMU.c 2011-01-28 01:14:37.000000000 +0100
> +++ ulogd-2.0.0/output/ulogd_output_LOGEMU.c 2012-09-05 15:23:26.000000000 +0200
> @@ -7,6 +7,11 @@
> *
> * (C) 2000-2005 by Harald Welte <laforge@xxxxxxxxxxxx>
> *
> + * 05 Sep 2012, Gerfried Essler <essler@xxxxxxxxxx>,
> + * Sven Anders <anders@xxxxxxxxxx>:
> + * Added macros (for date/time) in filename/directories and
> + * automatic creation of it. Additional behaviour like syslog-ng.
These credits will show up in the git changelog, not good to add
history there. You may still add your copyright if you want.
> + *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2
> * as published by the Free Software Foundation
> @@ -30,6 +35,10 @@
> #include <string.h>
> #include <errno.h>
> #include <time.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> #include <ulogd/ulogd.h>
> #include <ulogd/conffile.h>
>
> @@ -58,10 +67,26 @@
> .flags = ULOGD_KEYF_OPTIONAL,
> .name = "oob.time.sec",
> },
> +#ifdef LOGNAME_EXPANSION
> + {
> + .type = ULOGD_RET_UINT32,
> + .flags = ULOGD_KEYF_OPTIONAL,
> + .name = "local.time",
> + },
> + {
> + .type = ULOGD_RET_STRING,
> + .flags = ULOGD_KEYF_OPTIONAL,
> + .name = "local.hostname",
> + },
> +#endif /*LOGNAME_EXPANSION*/
> };
>
> static struct config_keyset logemu_kset = {
> +#ifdef LOGNAME_EXPANSION
> + .num_ces = 9,
> +#else
> .num_ces = 2,
> +#endif /*LOGNAME_EXPANSION*/
> .ces = {
> {
> .key = "file",
> @@ -75,13 +100,155 @@
> .options = CONFIG_OPT_NONE,
> .u = { .value = ULOGD_LOGEMU_SYNC_DEFAULT },
> },
> +#ifdef LOGNAME_EXPANSION
> + {
> + .key = "create_dirs",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = 0 },
> + },
> + {
> + .key = "uid",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = -1 },
> + },
> + {
> + .key = "gid",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = -1 },
> + },
> + {
> + .key = "dir_uid",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = -1 },
> + },
> + {
> + .key = "dir_gid",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = -1 },
> + },
> + {
> + .key = "mode",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = 0400 },
> + },
> + {
> + .key = "dir_mode",
> + .type = CONFIG_TYPE_INT,
> + .options = CONFIG_OPT_NONE,
> + .u = { .value = 0700 },
> + },
> +
> +#endif /*LOGNAME_EXPANSION*/
> },
> };
>
> struct logemu_instance {
> FILE *of;
> +#ifdef LOGNAME_EXPANSION
> + int macros;
> + char filename[PATH_MAX];
> +#endif /*LOGNAME_EXPANSION*/
> };
>
> +#ifdef LOGNAME_EXPANSION
> +
> +static int open_file(struct ulogd_pluginstance *upi, char *filename, FILE **fd)
> +{
> + /* Let ulog use unix file/folder privileges */
> + int create_dirs = 0;
> + int uid = 0;
> + int gid = 0;
> + int mode = 0;
> + int dir_uid = 0;
> + int dir_gid = 0;
> + int dir_mode = 0;
> + mode_t old_umask = 0;
> +
> + /* set variables for easier access */
> + create_dirs = upi->config_kset->ces[2].u.value;
> + uid = upi->config_kset->ces[3].u.value;
> + gid = upi->config_kset->ces[4].u.value;
> + dir_uid = upi->config_kset->ces[5].u.value;
> + dir_gid = upi->config_kset->ces[6].u.value;
> + mode = upi->config_kset->ces[7].u.value;
> + dir_mode = upi->config_kset->ces[8].u.value;
> +
> + if (filename == NULL)
> + return 0;
> +
> + old_umask = umask(0777);
> +
> + *fd = fopen(filename, "a");
> +
> + if (create_dirs && *fd == NULL && errno == ENOENT) {
On error, display a message and return -1.
You can avoid lots of nesting with that error treatment.
> +
> + /* directory does not exist */
> + char *p = filename + 1;
> + p = strchr(p, '/');
> +
> + while (p) {
> + struct stat st;
> + *p = 0;
> + if (stat(filename, &st) == 0) {
> +
I can see lots of whitespace errors, ie. empty lines with tabs.
Please, fix those.
> + if (!S_ISDIR(st.st_mode))
> + return 0;
> + } else if (errno == ENOENT) {
> +
> + if (mkdir(filename, dir_mode) == -1) {
> + ulogd_log(ULOGD_ERROR, "Couldn't create directory \"%s\": %s.\n",
> + filename, strerror(errno));
> + return 0;
> + }
> +
> + if (dir_uid != -1 || dir_gid != -1) {
> +
> + if (chown(filename, dir_uid, dir_gid) < 0) {
> + ulogd_log(ULOGD_NOTICE, "Couldn't set owner on directory \"%s\": %s.\n",
> + filename, strerror(errno));
> +
> + }
> + }
> +
> + if (chmod(filename, dir_mode) < 0) {
> + ulogd_log(ULOGD_NOTICE, "Couldn't set permissions on directory \"%s\": %s.\n",
> + filename, strerror(errno));
> + }
> + }
> +
> + *p = '/';
> + p = strchr(p + 1, '/');
> + }
> +
> + *fd = fopen(filename, "a");
> + }
> +
> + if (uid != -1 || gid != -1) {
> +
> + if (chown(filename, uid, gid) < 0) {
> + ulogd_log(ULOGD_NOTICE, "Couldn't set owner on file \"%s\": %s.\n",
> + filename, strerror(errno));
> + }
> + }
> +
> + if (chmod(filename, mode) < 0) {
> + ulogd_log(ULOGD_NOTICE, "Couldn't set permissions on file \"%s\": %s.\n",
> + filename, strerror(errno));
> + }
> +
> + umask(old_umask);
> +
> + return (*fd != NULL);
> +}
> +
> +#endif /*LOGNAME_EXPANSION*/
> +
> static int _output_logemu(struct ulogd_pluginstance *upi)
> {
> struct logemu_instance *li = (struct logemu_instance *) &upi->private;
> @@ -100,12 +267,69 @@
> timestr = ctime(&now) + 4;
> if ((tmp = strchr(timestr, '\n')))
> *tmp = '\0';
> +
> +#ifdef LOGNAME_EXPANSION
> +
> + char new_filename[PATH_MAX];
> + const struct tm *tm;
> + /* convert time to tm structure */
> + tm = localtime(&now);
> +
> + // This contains the time macro path string the user has configured
> + char* formatstring = upi->config_kset->ces[0].u.string;
We have to break lines at 80-chars.
And we use C comments, /* ... */, not C++ comments //
> +
> + if (li->macros) { // do we use macros?
You can put this code below in some function, so you can save some
nesting levels. That's also good for code maintainability.
> +
> + // expand any macros in the file's name
> + if (strftime(new_filename, PATH_MAX-1, formatstring, tm) > 0) {
> +
> + // Do we need to open another log file?
> + if ((li->filename == NULL) ||
> + (strcmp(new_filename, li->filename) != 0)) {
> +
> + // close the old file
> + if (li->filename != NULL) {
> + if (li->of != NULL) fclose(li->of);
> + memset(li->filename, 0, PATH_MAX);
> + }
> +
> + // copy new filename and create path, if necessary...
> + memcpy(li->filename, new_filename, PATH_MAX);
> + ulogd_log(ULOGD_INFO, "Opening new logfile '%s'.\n",
> + li->filename);
> +
> + // open the new file
> + if (!open_file(upi, li->filename, &li->of)) {
> + ulogd_log(ULOGD_FATAL, "Couldn't open \"%s\": %s.\n",
> + li->filename, strerror(errno));
> + return -1;
> + }
> + }
> + } else if (li->filename[0] == '\0')
> + return -1;
> + }
> +
> +#ifdef DEBUG_LOGEMU
> + printf("%d:%d:%d %s %s",tm->tm_hour,tm->tm_min,
> + tm->tm_sec,hostname, (char *) res[0].u.source->u.value.ptr);
> +#endif /*DEBUG_LOGEMU*/
> +
> + /* Finally write our log to the file */
> + fprintf(li->of, "%d:%d:%d %s %s",tm->tm_hour,tm->tm_min,
> + tm->tm_sec,hostname,
> + (char *) res[0].u.source->u.value.ptr);
> +
> + fflush(li->of);
> +
> +#else
>
> fprintf(li->of, "%.15s %s %s", timestr, hostname,
> (char *) res[0].u.source->u.value.ptr);
> +
> + if (upi->config_kset->ces[1].u.value)
> + fflush(li->of);
>
> - if (upi->config_kset->ces[1].u.value)
> - fflush(li->of);
> +#endif /*LOGNAME_EXPANSION*/
> }
>
> return ULOGD_IRET_OK;
> @@ -144,14 +368,31 @@
> #ifdef DEBUG_LOGEMU
> li->of = stdout;
> #else
> +#ifdef LOGNAME_EXPANSION
> +
> + li->macros = 0;
> + memset(li->filename, 0, PATH_MAX);
> +
> +#endif /*LOGEMU_EXPANSION*/
> +
> ulogd_log(ULOGD_DEBUG, "opening file: %s\n",
> pi->config_kset->ces[0].u.string);
> +#ifdef LOGNAME_EXPANSION
> + if (strchr(pi->config_kset->ces[0].u.string, '%') == NULL) {
> +#endif /*LOGNAME_EXPANSION*/
> li->of = fopen(pi->config_kset->ces[0].u.string, "a");
> if (!li->of) {
> ulogd_log(ULOGD_FATAL, "can't open syslogemu: %s\n",
> strerror(errno));
> return -errno;
> - }
> +}
> +#ifdef LOGNAME_EXPANSION
> + } else {
> + li->of = NULL;
> + li->macros = 1;
> + }
> +#endif /*LOGNAME_EXPANSION*/
> +
> #endif
>
> if (gethostname(hostname, sizeof(hostname)) < 0) {
> @@ -170,8 +411,15 @@
> static int fini_logemu(struct ulogd_pluginstance *pi) {
> struct logemu_instance *li = (struct logemu_instance *) &pi->private;
>
> +#ifdef LOGNAME_EXPANSION
> +
> + if (li->of != stdout && li->of != NULL)
> + fclose(li->of);
> +#else
> +
> if (li->of != stdout)
> fclose(li->of);
> +#endif /*LOGNAME_EXPANSION*/
>
> return 0;
> }
--- End Message ---
