[ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Arturo Borrero Gonzalez]

This new configuration option eases a bit the configuration of ulogd2 by
allowing to load all plugins in one go, without having to know their full path.

Choosing concrete plugins and using full path for them is great for some
environmnets, but I don't think it's a common case. The common case is to
load all plugins, even ignoring where do they live in the filesystem.

Even worse, the full path may be architecture-dependant, which makes copying
the ulogd.conf file between machines unnecesarily complex.

There are two ways of using this new config directive:

1) leave it empty	(i.e. 'load_all_plugins=')
2) use a path		(i.e. 'load_all_plugins='/usr/local/lib/mydir/')

In the first case, plugins will be loaded from a default directory, choosen at
build/configure time (--with-ulogd2libdir). If no specified, this is something
like '/usr/local/lib/ulogd/'.

In the second case, the user is responsible of providing a sensible path.

The 'load_all_plugins' directive may be combined with the old 'plugin'
directive to load other custom-made plugins elsewhere, like always.
The 'plugin' directive is keep unchanged.

This new configuration option doesn't implement any special logic. We simply
open the dir and try to load all files ending with '.so'.

Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
 configure.ac  |   30 +++++++++++++++++++++++++++---
 src/ulogd.c   |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 ulogd.conf.in |   10 ++++++++++
 3 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index e661981..b3441e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -36,9 +36,6 @@ dnl Checks for library functions.
 AC_FUNC_VPRINTF
 AC_CHECK_FUNCS(socket strerror)
 
-regular_CFLAGS="-Wall -Wextra -Wno-unused-parameter"
-AC_SUBST([regular_CFLAGS])
-
 AC_SEARCH_LIBS([pthread_create], [pthread], [libpthread_LIBS="$LIBS"; LIBS=""])
 AC_SUBST([libpthread_LIBS])
 
@@ -153,6 +150,16 @@ else
 	enable_jansson="no"
 fi
 
+AC_ARG_WITH([ulogd2libdir],
+	AS_HELP_STRING([--with-ulogd2libdir=PATH],
+        [Default directory to load ulogd2 plugin from [[LIBDIR/ulogd]]]),
+        [ulogd2libdir="$withval"],
+        [ulogd2libdir="${libdir}/ulogd"])
+AC_SUBST([ulogd2libdir])
+
+regular_CFLAGS="-Wall -Wextra -Wno-unused-parameter -DULOGD2_LIBDIR=\\\"\${ulogd2libdir}\\\"";
+AC_SUBST([regular_CFLAGS])
+
 dnl AC_SUBST(DATABASE_DIR)
 dnl AC_SUBST(DATABASE_LIB)
 dnl AC_SUBST(DATABASE_LIB_DIR)
@@ -176,8 +183,25 @@ AC_CONFIG_FILES(include/Makefile include/ulogd/Makefile include/libipulog/Makefi
 	  src/Makefile Makefile Rules.make)
 AC_OUTPUT
 
+define([EXPAND_VARIABLE],
+[$2=[$]$1
+if test $prefix = 'NONE'; then
+        prefix="/usr/local"
+fi
+while true; do
+  case "[$]$2" in
+    *\[$]* ) eval "$2=[$]$2" ;;
+    *) break ;;
+  esac
+done
+eval "$2=[$]$2"
+])dnl EXPAND_VARIABLE
+
+EXPAND_VARIABLE(ulogd2libdir, e_ulogd2libdir)
+
 echo "
 Ulogd configuration:
+  Default plugins directory:		${e_ulogd2libdir}
   Input plugins:
     NFLOG plugin:			${enable_nflog}
     NFCT plugin:			${enable_nfct}
diff --git a/src/ulogd.c b/src/ulogd.c
index 684444f..5ae8498 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -124,10 +124,11 @@ static LLIST_HEAD(ulogd_pi_stacks);
 static int load_plugin(const char *file);
 static int create_stack(const char *file);
 static int logfile_open(const char *name);
+static int load_all_plugins();
 static void cleanup_pidfile();
 
 static struct config_keyset ulogd_kset = {
-	.num_ces = 4,
+	.num_ces = 5,
 	.ces = {
 		{
 			.key = "logfile",
@@ -153,6 +154,12 @@ static struct config_keyset ulogd_kset = {
 			.options = CONFIG_OPT_MULTI,
 			.u.parser = &create_stack,
 		},
+		{
+			.key = "load_all_plugins",
+			.type = CONFIG_TYPE_CALLBACK,
+			.options = CONFIG_OPT_NONE,
+			.u.parser = &load_all_plugins,
+		},
 	},
 };
 
@@ -728,6 +735,46 @@ static int load_plugin(const char *file)
 	return 0;
 }
 
+static int load_all_plugins(const char *arg)
+{
+	DIR *d;
+	struct dirent *dent;
+	char path[PATH_MAX];
+	const char *dir;
+
+	if (strcmp(arg, "load_all_plugins") == 0) /* no argument in conf */
+		dir = ULOGD2_LIBDIR;
+	else
+		dir = arg;
+
+	d = opendir(dir);
+	if (d == NULL) {
+		ulogd_log(ULOGD_ERROR, "load_all_plugins: opendir(%s): %s\n",
+			  dir, strerror(errno));
+		return -1;
+	}
+
+	ulogd_log(ULOGD_NOTICE, "loading all plugins at %s\n", dir);
+
+	while ((dent = readdir(d)) != NULL) {
+		if (strcmp(dent->d_name, ".") == 0 ||
+		    strcmp(dent->d_name, "..") == 0)
+			continue;
+
+		int len = strlen(dent->d_name);
+		if (len < 3)
+			continue;
+
+		if (strcmp(&dent->d_name[len - 3], ".so") != 0)
+			continue;
+
+		snprintf(path, sizeof(path), "%s/%s", dir, dent->d_name);
+		if (load_plugin(path) != 0)
+			return -1;
+	}
+	return 0;
+}
+
 /* find an output key in a given stack, starting at 'start' */
 static struct ulogd_key *
 find_okey_in_stack(char *name,
diff --git a/ulogd.conf.in b/ulogd.conf.in
index a987d64..fe54420 100644
--- a/ulogd.conf.in
+++ b/ulogd.conf.in
@@ -24,6 +24,16 @@ logfile="/var/log/ulogd.log"
 # 2. options for each plugin in seperate section below
 
 
+# load all the plugins in one go. Then, there is no need to specify each
+# plugin individually. There are two ways of using this clause, by leaving it
+# blank (default) or by using a filesystem path. If blank a default directory
+# configured at build time will be used (--with-ulogd2libdir).
+#
+# Examples:
+#
+# load_all_plugins=
+# load_all_plugins=/usr/local/lib/ulogd/
+
 plugin="@pkglibdir@/ulogd_inppkt_NFLOG.so"
 #plugin="@pkglibdir@/ulogd_inppkt_ULOG.so"
 #plugin="@pkglibdir@/ulogd_inppkt_UNIXSOCK.so"

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Pablo Neira Ayuso]

Hi Arturo,

On Mon, Sep 25, 2017 at 01:19:27PM +0200, Arturo Borrero Gonzalez wrote:
> diff --git a/ulogd.conf.in b/ulogd.conf.in
> index a987d64..fe54420 100644
> --- a/ulogd.conf.in
> +++ b/ulogd.conf.in
> @@ -24,6 +24,16 @@ logfile="/var/log/ulogd.log"
>  # 2. options for each plugin in seperate section below
>  
>  
> +# load all the plugins in one go. Then, there is no need to specify each
> +# plugin individually. There are two ways of using this clause, by leaving it
> +# blank (default) or by using a filesystem path. If blank a default directory
> +# configured at build time will be used (--with-ulogd2libdir).
> +#
> +# Examples:
> +#
> +# load_all_plugins=
> +# load_all_plugins=/usr/local/lib/ulogd/
> +
>  plugin="@pkglibdir@/ulogd_inppkt_NFLOG.so"
>  #plugin="@pkglibdir@/ulogd_inppkt_ULOG.so"
>  #plugin="@pkglibdir@/ulogd_inppkt_UNIXSOCK.so"

Just an idea, probably better something like:

        plugin="@pkglibdir@/"

I mean, if you specify a directory, then this means "include every
ulogd_*.so file there", it's easy to check via stat() if this is path
represents a directory, so you can skip string handling tricks.

Thanks.

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Arturo Borrero Gonzalez]

On 29 September 2017 at 13:39, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Arturo,
>
> On Mon, Sep 25, 2017 at 01:19:27PM +0200, Arturo Borrero Gonzalez wrote:
>> diff --git a/ulogd.conf.in b/ulogd.conf.in
>> index a987d64..fe54420 100644
>> --- a/ulogd.conf.in
>> +++ b/ulogd.conf.in
>> @@ -24,6 +24,16 @@ logfile="/var/log/ulogd.log"
>>  # 2. options for each plugin in seperate section below
>>
>>
>> +# load all the plugins in one go. Then, there is no need to specify each
>> +# plugin individually. There are two ways of using this clause, by leaving it
>> +# blank (default) or by using a filesystem path. If blank a default directory
>> +# configured at build time will be used (--with-ulogd2libdir).
>> +#
>> +# Examples:
>> +#
>> +# load_all_plugins=
>> +# load_all_plugins=/usr/local/lib/ulogd/
>> +
>>  plugin="@pkglibdir@/ulogd_inppkt_NFLOG.so"
>>  #plugin="@pkglibdir@/ulogd_inppkt_ULOG.so"
>>  #plugin="@pkglibdir@/ulogd_inppkt_UNIXSOCK.so"
>
> Just an idea, probably better something like:
>
>         plugin="@pkglibdir@/"
>
> I mean, if you specify a directory, then this means "include every
> ulogd_*.so file there", it's easy to check via stat() if this is path
> represents a directory, so you can skip string handling tricks.

Ok, but how could we avoid putting there a complex, arch-dependant path?
My first idea was to have the new config directive to don't accept any
path and having the default set at build/configure time.
For the sake of flexibility I added in the last moment the option for
the user to give a path and override the one set at build/configure
time.

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Arturo Borrero Gonzalez]

On 30 September 2017 at 11:43, Arturo Borrero Gonzalez
<arturo@netfilter.org> wrote:
>
> Ok, but how could we avoid putting there a complex, arch-dependant path?

i.e, in Debian this means a path like:

/usr/lib/mips64el-linux-gnuabi64/ulogd/ulogd_filter_IFINDEX.so

so user should use /usr/lib/mips64el-linux-gnuabi64/ which is very ugly.
If the config file is copied to a machine with a different arch, amd64
for example, then path should be modified to:

/usr/lib/x86_64-linux-gnu/ulogd/

Complex and ugly. We should avoid that. I think we should offer a
default at build/configure time.

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Pablo Neira Ayuso]

On Sat, Sep 30, 2017 at 11:48:11AM +0200, Arturo Borrero Gonzalez wrote:
> On 30 September 2017 at 11:43, Arturo Borrero Gonzalez
> <arturo@netfilter.org> wrote:
> >
> > Ok, but how could we avoid putting there a complex, arch-dependant path?
> 
> i.e, in Debian this means a path like:
> 
> /usr/lib/mips64el-linux-gnuabi64/ulogd/ulogd_filter_IFINDEX.so
> 
> so user should use /usr/lib/mips64el-linux-gnuabi64/ which is very ugly.
> If the config file is copied to a machine with a different arch, amd64
> for example, then path should be modified to:
> 
> /usr/lib/x86_64-linux-gnu/ulogd/
> 
> Complex and ugly. We should avoid that. I think we should offer a
> default at build/configure time.

I think @pkglibdir@ in ulogd.conf.in will set this to the
corresponding arch-dependent folder at configure/build time, right?

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Arturo Borrero Gonzalez]

On 30 September 2017 at 12:12, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Sep 30, 2017 at 11:48:11AM +0200, Arturo Borrero Gonzalez wrote:
>> On 30 September 2017 at 11:43, Arturo Borrero Gonzalez
>> <arturo@netfilter.org> wrote:
>> >
>> > Ok, but how could we avoid putting there a complex, arch-dependant path?
>>
>> i.e, in Debian this means a path like:
>>
>> /usr/lib/mips64el-linux-gnuabi64/ulogd/ulogd_filter_IFINDEX.so
>>
>> so user should use /usr/lib/mips64el-linux-gnuabi64/ which is very ugly.
>> If the config file is copied to a machine with a different arch, amd64
>> for example, then path should be modified to:
>>
>> /usr/lib/x86_64-linux-gnu/ulogd/
>>
>> Complex and ugly. We should avoid that. I think we should offer a
>> default at build/configure time.
>
> I think @pkglibdir@ in ulogd.conf.in will set this to the
> corresponding arch-dependent folder at configure/build time, right?

The point is to don't have the ugly string in the config file.
Transparent to the user. Simplify the config file.

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Pablo Neira Ayuso]

On Sat, Sep 30, 2017 at 12:43:36PM +0200, Arturo Borrero Gonzalez wrote:
> On 30 September 2017 at 12:12, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sat, Sep 30, 2017 at 11:48:11AM +0200, Arturo Borrero Gonzalez wrote:
> >> On 30 September 2017 at 11:43, Arturo Borrero Gonzalez
> >> <arturo@netfilter.org> wrote:
> >> >
> >> > Ok, but how could we avoid putting there a complex, arch-dependant path?
> >>
> >> i.e, in Debian this means a path like:
> >>
> >> /usr/lib/mips64el-linux-gnuabi64/ulogd/ulogd_filter_IFINDEX.so
> >>
> >> so user should use /usr/lib/mips64el-linux-gnuabi64/ which is very ugly.
> >> If the config file is copied to a machine with a different arch, amd64
> >> for example, then path should be modified to:
> >>
> >> /usr/lib/x86_64-linux-gnu/ulogd/
> >>
> >> Complex and ugly. We should avoid that. I think we should offer a
> >> default at build/configure time.
> >
> > I think @pkglibdir@ in ulogd.conf.in will set this to the
> > corresponding arch-dependent folder at configure/build time, right?
> 
> The point is to don't have the ugly string in the config file.
> Transparent to the user. Simplify the config file.

OK.

What if we default to loading all plugins if user specifies no
"plugin=" at all in the configuration file?

No worries in terms of breaking backward compatibility, so far ulogd2
just bails out if no plugin is available.

That would simplify the configuration file as you're searching for.

Re: [ulogd2 PATCH] ulogd2: add new config option: load_all_plugins

[Arturo Borrero Gonzalez]

On 2 October 2017 at 12:44, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Sep 30, 2017 at 12:43:36PM +0200, Arturo Borrero Gonzalez wrote:
>> On 30 September 2017 at 12:12, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Sat, Sep 30, 2017 at 11:48:11AM +0200, Arturo Borrero Gonzalez wrote:
>> >> On 30 September 2017 at 11:43, Arturo Borrero Gonzalez
>> >> <arturo@netfilter.org> wrote:
>> >> >
>> >> > Ok, but how could we avoid putting there a complex, arch-dependant path?
>> >>
>> >> i.e, in Debian this means a path like:
>> >>
>> >> /usr/lib/mips64el-linux-gnuabi64/ulogd/ulogd_filter_IFINDEX.so
>> >>
>> >> so user should use /usr/lib/mips64el-linux-gnuabi64/ which is very ugly.
>> >> If the config file is copied to a machine with a different arch, amd64
>> >> for example, then path should be modified to:
>> >>
>> >> /usr/lib/x86_64-linux-gnu/ulogd/
>> >>
>> >> Complex and ugly. We should avoid that. I think we should offer a
>> >> default at build/configure time.
>> >
>> > I think @pkglibdir@ in ulogd.conf.in will set this to the
>> > corresponding arch-dependent folder at configure/build time, right?
>>
>> The point is to don't have the ugly string in the config file.
>> Transparent to the user. Simplify the config file.
>
> OK.
>
> What if we default to loading all plugins if user specifies no
> "plugin=" at all in the configuration file?
>
> No worries in terms of breaking backward compatibility, so far ulogd2
> just bails out if no plugin is available.
>
> That would simplify the configuration file as you're searching for.

Ok, will do that.