Re: [PATCH v2] add systemd service file

Re: [PATCH v2] add systemd service file

[Jörg Thalheim]

[-- Attachment #1: Type: text/plain, Size: 4884 bytes --]

The last PATCH was rejected, because it has added an integration script.
This PATCH however only adds a service file, with no other dependency but the
userpace nft program.

my motivation was the following:

- Providing a service file upstream hopefully lead to consistent behaviour across distributions
- The people, who know how to deal with nft, are usually the upstream developer itself
- The provided service should be reusable without any modification in any distribution 
  and should preserve maintainers from reinventing the wheel all the time 
  (debian currently does not provide atomic reloading in sid for example; 
  archlinux does, but it doesn't set ProtectSystem and ProtectHome)

I hope you can agree with this. Thanks

On Wed, 17 Dec 2014 17:08:46 +0100
Jörg Thalheim <joerg@higgsboson.tk> wrote:

> Signed-off-by: Jörg Thalheim <joerg@higgsboson.tk>
> ---
>  configure.ac                   | 30 +++++++++++++++++++++++++++++-
>  files/Makefile.am              |  3 ++-
>  files/nftables/nftables.conf   |  0
>  files/systemd/Makefile.am      |  7 +++++++
>  files/systemd/nftables.service | 17 +++++++++++++++++
>  5 files changed, 55 insertions(+), 2 deletions(-)
>  create mode 100644 files/nftables/nftables.conf
>  create mode 100644 files/systemd/Makefile.am
>  create mode 100644 files/systemd/nftables.service
> 
> diff --git a/configure.ac b/configure.ac
> index d8f949a..f4352a6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -13,6 +13,8 @@ AC_CONFIG_MACRO_DIR([m4])
>  AM_INIT_AUTOMAKE([-Wall foreign subdir-objects
>          tar-pax no-dist-gzip dist-bzip2 1.6])
>  
> +AC_PATH_TOOL(PKGCONFIG, pkg-config)
> +
>  dnl kernel style compile messages
>  m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])
>  
> @@ -117,6 +119,30 @@ AC_TYPE_UINT16_T
>  AC_TYPE_UINT32_T
>  AC_TYPE_UINT64_T
>  
> +AC_ARG_WITH(systemd, [  --with-systemd          set directory for
> systemd service files],
> +        [systemd_unitdir="$withval"; with_systemd=yes],
> +        [systemd_unitdir=""; with_systemd=no])
> +AC_SUBST(systemd_unitdir)
> +
> +AM_CONDITIONAL([INSTALL_SYSTEMD], [test "x$with_systemd" != xno])
> +AM_COND_IF([INSTALL_SYSTEMD],
> +       [AS_IF([test "x$PKGCONFIG" = "x"],
> +             [AC_MSG_ERROR(Need pkg-config to enable systemd
> support.)], +
> +             [AC_MSG_CHECKING(for systemd)
> +              AS_IF([$PKGCONFIG --exists systemd],
> +                    [AC_MSG_RESULT(yes)
> +                     AS_IF([$PKGCONFIG --exists systemd],
> +                           [AS_IF([test "x$systemd_unit_dir" = "x"],
> +                                  [ systemd_unitdir="`$PKGCONFIG
> --variable=systemdsystemunitdir systemd`"])
> +                           ])
> +                    ]
> +                    [AC_MSG_RESULT(no)])
> +             ]
> +
> +       )]
> +)
> +
>  # Checks for library functions.
>  AC_CHECK_FUNCS([memmove memset strchr strdup strerror strtoull])
>  
> @@ -129,6 +155,7 @@
> AC_CONFIG_FILES([					\
> doc/Makefile				\
> files/Makefile				\
> files/nftables/Makefile			\
> +		files/systemd/Makefile			\
>  		])
>  AC_OUTPUT
>  
> @@ -136,4 +163,5 @@ echo "
>  nft configuration:
>    cli support:			${with_cli}
>    enable debugging:		${with_debug}
> -  use mini-gmp:			${with_mini_gmp}"
> +  use mini-gmp:			${with_mini_gmp}
> +  systemd support:		${with_systemd}"
> diff --git a/files/Makefile.am b/files/Makefile.am
> index a8394c0..4dc0027 100644
> --- a/files/Makefile.am
> +++ b/files/Makefile.am
> @@ -1 +1,2 @@
> -SUBDIRS = nftables
> +SUBDIRS =	nftables	\
> +					systemd
> diff --git a/files/nftables/nftables.conf
> b/files/nftables/nftables.conf new file mode 100644
> index 0000000..e69de29
> diff --git a/files/systemd/Makefile.am b/files/systemd/Makefile.am
> new file mode 100644
> index 0000000..2bf8580
> --- /dev/null
> +++ b/files/systemd/Makefile.am
> @@ -0,0 +1,7 @@
> +if INSTALL_SYSTEMD
> +systemd_unit_DATA = nftables.service
> +
> +install-data-hook:
> +	${SED} -i
> 's|@sbindir[@]|${sbindir}/|g;s|@sysconfdir[@]|${sysconfdir}/|g' \
> +		${DESTDIR}${systemd_unitdir}/nftables.service
> +endif
> diff --git a/files/systemd/nftables.service
> b/files/systemd/nftables.service new file mode 100644
> index 0000000..bdb12cf
> --- /dev/null
> +++ b/files/systemd/nftables.service
> @@ -0,0 +1,17 @@
> +[Unit]
> +Description=Netfilter Tables
> +Documentation=man:nft(8)
> +Wants=network-pre.target
> +Before=network-pre.target
> +
> +[Service]
> +Type=oneshot
> +ProtectSystem=full
> +ProtectHome=true
> +ExecStart=@sbindir@nft -f /etc/nftables.conf
> +ExecReload=@sbindir@nft 'flush ruleset; include
> "/etc/nftables.conf";' +ExecStop=@sbindir@nft flush ruleset
> +RemainAfterExit=yes
> +
> +[Install]
> +WantedBy=multi-user.target


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: [PATCH v2] add systemd service file

[Arturo Borrero Gonzalez]

On 20 March 2015 at 10:53, Jörg Thalheim <joerg@higgsboson.tk> wrote:
> The last PATCH was rejected, because it has added an integration script.
> This PATCH however only adds a service file, with no other dependency but the
> userpace nft program.
>

Again, my opinion is: this belongs to distributions.

> my motivation was the following:
>
> - Providing a service file upstream hopefully lead to consistent behaviour across distributions
> - The people, who know how to deal with nft, are usually the upstream developer itself
> - The provided service should be reusable without any modification in any distribution
>   and should preserve maintainers from reinventing the wheel all the time
>   (debian currently does not provide atomic reloading in sid for example;
>   archlinux does, but it doesn't set ProtectSystem and ProtectHome)
>

It turns out that the debian package do reload the ruleset atomically.
We can further discuss the debian stuff in other place though.

-- 
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: [PATCH v2] add systemd service file

[Patrick McHardy]

On 20.03, Jörg Thalheim wrote:
> The last PATCH was rejected, because it has added an integration script.
> This PATCH however only adds a service file, with no other dependency but the
> userpace nft program.
> 
> my motivation was the following:
> 
> - Providing a service file upstream hopefully lead to consistent behaviour across distributions
> - The people, who know how to deal with nft, are usually the upstream developer itself
> - The provided service should be reusable without any modification in any distribution 
>   and should preserve maintainers from reinventing the wheel all the time 
>   (debian currently does not provide atomic reloading in sid for example; 
>   archlinux does, but it doesn't set ProtectSystem and ProtectHome)
> 
> I hope you can agree with this. Thanks

I'm think this is a good idea. However I don't know much about systemd
myself so please take care of Jan's suggestions first.
--
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

[PATCH v2] add systemd service file

[Jörg Thalheim]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

Signed-off-by: Jörg Thalheim <joerg@higgsboson.tk>
---
 configure.ac                   | 30 +++++++++++++++++++++++++++++-
 files/Makefile.am              |  3 ++-
 files/nftables/nftables.conf   |  0
 files/systemd/Makefile.am      |  7 +++++++
 files/systemd/nftables.service | 17 +++++++++++++++++
 5 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100644 files/nftables/nftables.conf
 create mode 100644 files/systemd/Makefile.am
 create mode 100644 files/systemd/nftables.service

diff --git a/configure.ac b/configure.ac
index d8f949a..f4352a6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,6 +13,8 @@ AC_CONFIG_MACRO_DIR([m4])
 AM_INIT_AUTOMAKE([-Wall foreign subdir-objects
         tar-pax no-dist-gzip dist-bzip2 1.6])
 
+AC_PATH_TOOL(PKGCONFIG, pkg-config)
+
 dnl kernel style compile messages
 m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])
 
@@ -117,6 +119,30 @@ AC_TYPE_UINT16_T
 AC_TYPE_UINT32_T
 AC_TYPE_UINT64_T
 
+AC_ARG_WITH(systemd, [  --with-systemd          set directory for systemd service files],
+        [systemd_unitdir="$withval"; with_systemd=yes],
+        [systemd_unitdir=""; with_systemd=no])
+AC_SUBST(systemd_unitdir)
+
+AM_CONDITIONAL([INSTALL_SYSTEMD], [test "x$with_systemd" != xno])
+AM_COND_IF([INSTALL_SYSTEMD],
+       [AS_IF([test "x$PKGCONFIG" = "x"],
+             [AC_MSG_ERROR(Need pkg-config to enable systemd support.)],
+
+             [AC_MSG_CHECKING(for systemd)
+              AS_IF([$PKGCONFIG --exists systemd],
+                    [AC_MSG_RESULT(yes)
+                     AS_IF([$PKGCONFIG --exists systemd],
+                           [AS_IF([test "x$systemd_unit_dir" = "x"],
+                                  [ systemd_unitdir="`$PKGCONFIG --variable=systemdsystemunitdir systemd`"])
+                           ])
+                    ]
+                    [AC_MSG_RESULT(no)])
+             ]
+
+       )]
+)
+
 # Checks for library functions.
 AC_CHECK_FUNCS([memmove memset strchr strdup strerror strtoull])
 
@@ -129,6 +155,7 @@ AC_CONFIG_FILES([					\
 		doc/Makefile				\
 		files/Makefile				\
 		files/nftables/Makefile			\
+		files/systemd/Makefile			\
 		])
 AC_OUTPUT
 
@@ -136,4 +163,5 @@ echo "
 nft configuration:
   cli support:			${with_cli}
   enable debugging:		${with_debug}
-  use mini-gmp:			${with_mini_gmp}"
+  use mini-gmp:			${with_mini_gmp}
+  systemd support:		${with_systemd}"
diff --git a/files/Makefile.am b/files/Makefile.am
index a8394c0..4dc0027 100644
--- a/files/Makefile.am
+++ b/files/Makefile.am
@@ -1 +1,2 @@
-SUBDIRS = nftables
+SUBDIRS =	nftables	\
+					systemd
diff --git a/files/nftables/nftables.conf b/files/nftables/nftables.conf
new file mode 100644
index 0000000..e69de29
diff --git a/files/systemd/Makefile.am b/files/systemd/Makefile.am
new file mode 100644
index 0000000..2bf8580
--- /dev/null
+++ b/files/systemd/Makefile.am
@@ -0,0 +1,7 @@
+if INSTALL_SYSTEMD
+systemd_unit_DATA = nftables.service
+
+install-data-hook:
+	${SED} -i 's|@sbindir[@]|${sbindir}/|g;s|@sysconfdir[@]|${sysconfdir}/|g' \
+		${DESTDIR}${systemd_unitdir}/nftables.service
+endif
diff --git a/files/systemd/nftables.service b/files/systemd/nftables.service
new file mode 100644
index 0000000..bdb12cf
--- /dev/null
+++ b/files/systemd/nftables.service
@@ -0,0 +1,17 @@
+[Unit]
+Description=Netfilter Tables
+Documentation=man:nft(8)
+Wants=network-pre.target
+Before=network-pre.target
+
+[Service]
+Type=oneshot
+ProtectSystem=full
+ProtectHome=true
+ExecStart=@sbindir@nft -f /etc/nftables.conf
+ExecReload=@sbindir@nft 'flush ruleset; include "/etc/nftables.conf";'
+ExecStop=@sbindir@nft flush ruleset
+RemainAfterExit=yes
+
+[Install]
+WantedBy=multi-user.target

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: [PATCH v2] add systemd service file

[Jan Engelhardt]

On Friday 2015-03-20 10:53, Jörg Thalheim wrote:
> 
>+AC_PATH_TOOL(PKGCONFIG, pkg-config)
>+

Sure you don't want to actually use PKG_PROG_PKG_CONFIG?

>@@ -0,0 +1,17 @@
>+[Unit]
>+Description=Netfilter Tables

That is rather non-descript. It should perhaps say something
involving "network packet filter".

>+[Install]
>+WantedBy=multi-user.target

This ought to be basic.target.
--
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