[RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Arturo Borrero Gonzalez]

This patch adds a basic systemd notification support.

Most of distros (Debian, RHEL, Ubuntu, ArchLinux...) use systemd as
init system.
Notifiying systemd that conntrackd is now running has many
benefits, the main being users concatenating systemd services depending on
the main conntrackd daemon being started.

The systemd support means conntrackd will require libsystemd, so a
configure swith is added:

 % ./configure --disable-systemd

We can further integrate conntrackd with systemd:
 * add watchdog support
 * report daemon errors (f.e, errno codes)
 * tell systemd conntrackd PID
 * report about conntrackd Unix socket

I've tested this against systemd 227.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
NOTE: the sd_notify() wrapper can be done in several other ways, f.e:
      https://github.com/MariaDB/server/blob/10.1/include/my_systemd.h

 configure.ac      |   12 +++++++++++-
 include/systemd.h |   10 ++++++++++
 src/Makefile.am   |    8 ++++++++
 src/main.c        |    3 +++
 src/systemd.c     |   25 +++++++++++++++++++++++++
 5 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 include/systemd.h
 create mode 100644 src/systemd.c

diff --git a/configure.ac b/configure.ac
index 70d3729..7c378b6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -61,6 +61,9 @@ AC_ARG_ENABLE([cthelper],
 AC_ARG_ENABLE([cttimeout],
         AS_HELP_STRING([--disable-cttimeout], [Do not build timeout support]),
         [enable_cttimeout="no"], [enable_cttimeout="yes"])
+AC_ARG_ENABLE([systemd],
+	AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
+	[enable_systemd="no"], [enable_systemd="yes"])
 
 PKG_CHECK_MODULES([LIBNFNETLINK], [libnfnetlink >= 1.0.1])
 PKG_CHECK_MODULES([LIBMNL], [libmnl >= 1.0.3])
@@ -77,6 +80,12 @@ AS_IF([test "x$enable_cthelper" = "xyes"], [
 ])
 AM_CONDITIONAL([HAVE_CTHELPER], [test "x$enable_cthelper" = "xyes"])
 
+AS_IF([test "x$enable_systemd" = "xyes"], [
+	PKG_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 227])
+	AC_DEFINE([BUILD_SYSTEMD], [1], [Building systemd support])
+])
+AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$enable_systemd" = "xyes"])
+
 AC_CHECK_HEADERS([linux/capability.h],, [AC_MSG_ERROR([Cannot find linux/capabibility.h])])
 
 # Checks for libraries.
@@ -146,4 +155,5 @@ AC_OUTPUT
 echo "
 conntrack-tools configuration:
   userspace conntrack helper support:	${enable_cthelper}
-  conntrack timeout support:		${enable_cttimeout}"
+  conntrack timeout support:		${enable_cttimeout}
+  systemd support:			${enable_systemd}"
diff --git a/include/systemd.h b/include/systemd.h
new file mode 100644
index 0000000..6e10b14
--- /dev/null
+++ b/include/systemd.h
@@ -0,0 +1,10 @@
+#ifndef _INCLUDE_SYSTEMD_H_
+#define _INCLUDE_SYSTEMD_H_
+
+void sd_ct_init(void);
+
+#ifndef BUILD_SYSTEMD
+void sd_ct_init(void){};
+#endif /* BUILD_SYSTEMD */
+
+#endif /* _INCLUDE_SYSTEMD_H_ */
diff --git a/src/Makefile.am b/src/Makefile.am
index a1d00f8..607f191 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -58,6 +58,10 @@ if HAVE_CTHELPER
 conntrackd_SOURCES += cthelper.c helpers.c utils.c expect.c
 endif
 
+if HAVE_SYSTEMD
+conntrackd_SOURCES += systemd.c
+endif
+
 # yacc and lex generate dirty code
 read_config_yy.o read_config_lex.o: AM_CFLAGS += -Wno-missing-prototypes -Wno-missing-declarations -Wno-implicit-function-declaration -Wno-nested-externs -Wno-undef -Wno-redundant-decls
 
@@ -68,6 +72,10 @@ if HAVE_CTHELPER
 conntrackd_LDADD += ${LIBNETFILTER_CTHELPER_LIBS} ${LIBNETFILTER_QUEUE_LIBS}
 endif
 
+if HAVE_SYSTEMD
+conntrackd_LDADD += ${LIBSYSTEMD_LIBS}
+endif
+
 conntrackd_LDFLAGS = -export-dynamic
 
 EXTRA_DIST = read_config_yy.h
diff --git a/src/main.c b/src/main.c
index dafeaee..9413db2 100644
--- a/src/main.c
+++ b/src/main.c
@@ -20,6 +20,7 @@
 #include "conntrackd.h"
 #include "log.h"
 #include "helper.h"
+#include "systemd.h"
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -422,6 +423,8 @@ int main(int argc, char *argv[])
 	} else
 		dlog(LOG_NOTICE, "-- starting in console mode --");
 
+	sd_ct_init();
+
 	/*
 	 * run main process
 	 */
diff --git a/src/systemd.c b/src/systemd.c
new file mode 100644
index 0000000..3210b9f
--- /dev/null
+++ b/src/systemd.c
@@ -0,0 +1,25 @@
+/*
+ * (C) 2015 by Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include "systemd.h"
+#include <systemd/sd-daemon.h>
+
+void sd_ct_init(void)
+{
+	sd_notify(0, "READY=1");
+}

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Jan Engelhardt]

On Friday 2015-10-16 14:10, Arturo Borrero Gonzalez wrote:

>+AC_ARG_ENABLE([systemd],
>+	AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
>+	[enable_systemd="no"], [enable_systemd="yes"])

This is incorrect. It needs to be

	[enable_systemd="$enableval"], [enable_systemd="yes"])


>+++ b/src/systemd.c
>@@ -0,0 +1,25 @@
>+/*
>+ * (C) 2015 by Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License as published by
>+ * the Free Software Foundation; either version 2 of the License, or
>+ * (at your option) any later version.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ * GNU General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program; if not, write to the Free Software
>+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>+ */
>+
>+#include "systemd.h"
>+#include <systemd/sd-daemon.h>
>+
>+void sd_ct_init(void)
>+{
>+	sd_notify(0, "READY=1");
>+}

It seems a bit excessive to create a new file just for this.
(In particular since it means you had to use AM_CONDITIONAL
to select the source file.)

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Arturo Borrero Gonzalez]

On 16 October 2015 at 16:10, Jan Engelhardt <jengelh@inai.de> wrote:
>
> On Friday 2015-10-16 14:10, Arturo Borrero Gonzalez wrote:
>
>>+AC_ARG_ENABLE([systemd],
>>+      AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
>>+      [enable_systemd="no"], [enable_systemd="yes"])
>
> This is incorrect. It needs to be
>
>         [enable_systemd="$enableval"], [enable_systemd="yes"])
>

I don't understand why. Could you please elaborate? The code above
(cthelper, cttimeout) is also bad?

I'm following this:
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Package-Options.html

>
>>+++ b/src/systemd.c
>>@@ -0,0 +1,25 @@
>>+/*
>>+ * (C) 2015 by Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>>+ *
>>+ * This program is free software; you can redistribute it and/or modify
>>+ * it under the terms of the GNU General Public License as published by
>>+ * the Free Software Foundation; either version 2 of the License, or
>>+ * (at your option) any later version.
>>+ *
>>+ * This program is distributed in the hope that it will be useful,
>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>+ * GNU General Public License for more details.
>>+ *
>>+ * You should have received a copy of the GNU General Public License
>>+ * along with this program; if not, write to the Free Software
>>+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>>+ */
>>+
>>+#include "systemd.h"
>>+#include <systemd/sd-daemon.h>
>>+
>>+void sd_ct_init(void)
>>+{
>>+      sd_notify(0, "READY=1");
>>+}
>
> It seems a bit excessive to create a new file just for this.
> (In particular since it means you had to use AM_CONDITIONAL
> to select the source file.)

Well, the idea (as stated in the patch description) is to add more
systemd-related features.
I'm thinking of:
 * tell systemd the state of the daemon in several circumstances
(start, stop, and so on)
 * tell systemd when the Unix socket is activated/deactivated
 * in the future, perhaps give support for systemd watchdog
 * other things... (PID, errno, etc...)

I guess a separate source file is OK for all the systemd-related stuff.

Sending READY=1 is just a basic starting feature. I don't want to
implement more things until it's clear we want systemd integration in
conntrackd.
-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Jan Engelhardt]

On Friday 2015-10-16 21:23, Arturo Borrero Gonzalez wrote:

>On 16 October 2015 at 16:10, Jan Engelhardt <jengelh@inai.de> wrote:
>>
>> On Friday 2015-10-16 14:10, Arturo Borrero Gonzalez wrote:
>>
>>>+AC_ARG_ENABLE([systemd],
>>>+      AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
>>>+      [enable_systemd="no"], [enable_systemd="yes"])
>>
>> This is incorrect. It needs to be
>>
>>         [enable_systemd="$enableval"], [enable_systemd="yes"])
>>
>
>I don't understand why. Could you please elaborate? The code above
>(cthelper, cttimeout) is also bad?
>
>https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Package-Options.html

Quoting the page:

— Macro: AC_ARG_ENABLE (feature, help-string, [action-if-given], 
[action-if-not-given])
    If the user gave configure the option --enable-feature or 
    --disable-feature, run shell commands action-if-given.


So

$ ./configure --enable-systemd

runs the 3rd argument, and always sets enable_systemd=no,
which is counter to what the user requested of configure.

In that regard, yes, conntrack-tools is also negatively affected. 
Possibly other software from netfilter.org too.



>Sending READY=1 is just a basic starting feature. I don't want to
>implement more things until it's clear we want systemd integration in
>conntrackd.

ok
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Pablo Neira Ayuso]

On Fri, Oct 16, 2015 at 11:37:31PM +0200, Jan Engelhardt wrote:
> On Friday 2015-10-16 21:23, Arturo Borrero Gonzalez wrote:
> 
> >On 16 October 2015 at 16:10, Jan Engelhardt <jengelh@inai.de> wrote:
> >>
> >> On Friday 2015-10-16 14:10, Arturo Borrero Gonzalez wrote:
> >>
> >>>+AC_ARG_ENABLE([systemd],
> >>>+      AS_HELP_STRING([--disable-systemd], [Do not build systemd support]),
> >>>+      [enable_systemd="no"], [enable_systemd="yes"])
> >>
> >> This is incorrect. It needs to be
> >>
> >>         [enable_systemd="$enableval"], [enable_systemd="yes"])
> >>
> >
> >I don't understand why. Could you please elaborate? The code above
> >(cthelper, cttimeout) is also bad?
> >
> >https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Package-Options.html
> 
> Quoting the page:
> 
> — Macro: AC_ARG_ENABLE (feature, help-string, [action-if-given], 
> [action-if-not-given])
>     If the user gave configure the option --enable-feature or 
>     --disable-feature, run shell commands action-if-given.
> 
> 
> So
> 
> $ ./configure --enable-systemd
> 
> runs the 3rd argument, and always sets enable_systemd=no,
> which is counter to what the user requested of configure.
> 
> In that regard, yes, conntrack-tools is also negatively affected. 
> Possibly other software from netfilter.org too.

Something like this (that we already have in nftables) should be fine:

commit 26b31033261427a3058a8fb994d54273bc93f7bf
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Tue Jul 7 14:32:21 2015 +0200

    configure: fix --enable-debug
    
    As the documentation indicates "The most common mistake for this
macro is to
    consider the two actions as action-if-enabled and
action-if-disabled."
    
    Use AS_IF in the action-if-present to check the real argument that
we're
    getting from the user.
    
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/configure.ac b/configure.ac
index d8f949a..931dbe1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -23,8 +23,8 @@ AC_DEFINE([_GNU_SOURCE], [], [Enable various GNU
extensions])
 AC_DEFINE([_STDC_FORMAT_MACROS], [], [printf-style format macros])
 
 AC_ARG_ENABLE([debug],
-             AS_HELP_STRING([--enable-debug], [Enable debugging]),
-             [with_debug=no],
+             AS_HELP_STRING([--enable-debug], [Disable debugging]),
+             AS_IF([test "x$enable_debug" = "xno"], [with_debug=no],
[with_debug=yes]),
              [with_debug=yes])
 AC_SUBST(with_debug)
 AM_CONDITIONAL([BUILD_DEBUG], [test "x$with_debug" != xno])

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Jan Engelhardt]

On Saturday 2015-10-17 12:57, Pablo Neira Ayuso wrote:
>Something like this (that we already have in nftables) should be fine:
>
> AC_ARG_ENABLE([debug],
>-             AS_HELP_STRING([--enable-debug], [Enable debugging]),
>-             [with_debug=no],
>+             AS_HELP_STRING([--enable-debug], [Disable debugging]),
>+             AS_IF([test "x$enable_debug" = "xno"], [with_debug=no],
>[with_debug=yes]),
>              [with_debug=yes])

Well the whole AS_IF part is not really needed, because you
are already testing in AM_CONDITIONAL:

> AC_SUBST(with_debug)
> AM_CONDITIONAL([BUILD_DEBUG], [test "x$with_debug" != xno])

Re: [RFC conntrackd PATCH] conntrackd: add basic systemd notification support

[Jan Engelhardt]

On Monday 2015-10-19 00:50, Jan Engelhardt wrote:
>On Saturday 2015-10-17 12:57, Pablo Neira Ayuso wrote:
>>Something like this (that we already have in nftables) should be fine:
>>
>> AC_ARG_ENABLE([debug],
>>-             AS_HELP_STRING([--enable-debug], [Enable debugging]),
>>-             [with_debug=no],
>>+             AS_HELP_STRING([--enable-debug], [Disable debugging]),
>>+             AS_IF([test "x$enable_debug" = "xno"], [with_debug=no],
>>[with_debug=yes]),
>>              [with_debug=yes])
>
>Well the whole AS_IF part is not really needed, because you
>are already testing in AM_CONDITIONAL:

That is to say, I find that something like the following would be
quite sufficient.


commit d0e677da000b1034df13718d4388ca78fbea97e6
Author: Jan Engelhardt <jengelh@inai.de>
Date:   Mon Oct 19 01:59:33 2015 +0200

    build: simplify parsing of --enable-debug

diff --git a/configure.ac b/configure.ac
index 0d7e6ed..780a3e6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -24,10 +24,8 @@ AC_DEFINE([_STDC_FORMAT_MACROS], [], [printf-style format macros])
 
 AC_ARG_ENABLE([debug],
 	      AS_HELP_STRING([--enable-debug], [Disable debugging]),
-	      AS_IF([test "x$enable_debug" = "xno"], [with_debug=no], [with_debug=yes]),
-	      [with_debug=yes])
-AC_SUBST(with_debug)
-AM_CONDITIONAL([BUILD_DEBUG], [test "x$with_debug" != xno])
+	      [enable_debug="$enableval"], [enable_debug=yes])
+AM_CONDITIONAL([BUILD_DEBUG], [test "x$enable_debug" != xno])
 
 # Checks for programs.
 AC_PROG_CC
@@ -135,5 +133,5 @@ AC_OUTPUT
 echo "
 nft configuration:
   cli support:			${with_cli}
-  enable debugging:		${with_debug}
+  enable debugging:		${enable_debug}
   use mini-gmp:			${with_mini_gmp}"