[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Nathaniel Roach]

We use a template service file as all of the daemons use almost
identical arguments and generally appear the same to the init
system.

We "Wants=" zebra as that's the daemon for interfacing to the
kernel, and it's not required for the other daemons to work
but it's probably going to be used in nearly all setups.

We need the helper script as systemd doesn't like having the
instance (%i) in the path to the executable.

Signed-off-by: Nathaniel Roach <nroach44@gmail.com>
---
 package/quagga/quagga-shim     | 51 ++++++++++++++++++++++++++++++++++++++++++
 package/quagga/quagga.mk       |  6 +++++
 package/quagga/quagga at .service | 19 ++++++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100644 package/quagga/quagga-shim
 create mode 100644 package/quagga/quagga at .service

diff --git a/package/quagga/quagga-shim b/package/quagga/quagga-shim
new file mode 100644
index 0000000..1687e7c
--- /dev/null
+++ b/package/quagga/quagga-shim
@@ -0,0 +1,51 @@
+#!/bin/sh
+#Shim for systemd's quagga at .service
+
+export QUAGGA_PATH="/usr/sbin"
+
+# Check if $1 is a valid quagga binary
+case $1 in
+  zebra)
+    export BINARY="zebra"
+    ;;
+
+  bgpd)
+    export BINARY="bgpd"
+    ;;
+
+  ospfd)
+    export BINARY="ospfd"
+    ;;
+
+  ospf6d)
+    export BINARY="bgpd"
+    ;;
+
+  ripd)
+    export BINARY="ripd"
+    ;;
+
+  ripngd)
+    export BINARY="ripngd"
+    ;;
+
+  pimd)
+    export BINARY="pimd"
+    ;;
+
+  *)
+    echo "Must be called with a valid Quagga daemon as the first argument"
+    exit 2
+    ;;
+
+esac
+
+# Check the binary exists
+if [ -e "$QUAGGA_PATH/$BINARY" ]; then
+  shift # Remove the first argument (binary name)
+  "$QUAGGA_PATH/$BINARY" "$@" # Run the daemon with the remaining arguments
+else
+  echo "Binary \"$BINARY\" not found in $QUAGGA_PATH"
+  exit 1
+fi
+
diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk
index 22e90ad..5d9c292 100644
--- a/package/quagga/quagga.mk
+++ b/package/quagga/quagga.mk
@@ -75,6 +75,12 @@ endif
 define QUAGGA_INSTALL_INIT_SYSTEMD
 	$(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \
 		$(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf
+
+	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
+		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service
+
+	$(INSTALL) -D -m 755 package/quagga/quagga-shim \
+		$(TARGET_DIR)/usr/sbin/quagga-shim
 endef
 
 $(eval $(autotools-package))
diff --git a/package/quagga/quagga at .service b/package/quagga/quagga at .service
new file mode 100644
index 0000000..3e98318
--- /dev/null
+++ b/package/quagga/quagga at .service
@@ -0,0 +1,19 @@
+[Unit]
+Description=Quagga %i routing daemon
+PartOf=quagga.service
+ReloadPropagatedFrom=quagga.service
+Wants=quagga at zebra.service
+
+[Service]
+PrivateTmp=true
+KillMode=mixed
+Type=forking
+EnvironmentFile=/etc/default/quagga-%i
+ExecStart=/usr/sbin/quagga-shim %i --daemon $OPTS -f /etc/quagga/%i.conf -i /var/run/quagga/%i.pid
+PIDFile=/var/run/quagga/%i.pid
+KillSignal=SIGINT
+Restart=on-failure
+RestartSec=1
+
+[Install]
+WantedBy=multi-user.target
-- 
2.8.1

[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Arnout Vandecappelle]

 Hi Nathaniel,

On 18-06-16 19:02, Nathaniel Roach wrote:
> We use a template service file as all of the daemons use almost
> identical arguments and generally appear the same to the init
> system.
> 
> We "Wants=" zebra as that's the daemon for interfacing to the
> kernel, and it's not required for the other daemons to work
> but it's probably going to be used in nearly all setups.

 ... and it can be overridden by fs-overlay.

> 
> We need the helper script as systemd doesn't like having the
> instance (%i) in the path to the executable.

 That last paragraph should IMHO be included as a comment in either the shim
itself or in the service file.

> 
> Signed-off-by: Nathaniel Roach <nroach44@gmail.com>
> ---
>  package/quagga/quagga-shim     | 51 ++++++++++++++++++++++++++++++++++++++++++
>  package/quagga/quagga.mk       |  6 +++++
>  package/quagga/quagga at .service | 19 ++++++++++++++++
>  3 files changed, 76 insertions(+)
>  create mode 100644 package/quagga/quagga-shim
>  create mode 100644 package/quagga/quagga at .service
> 
> diff --git a/package/quagga/quagga-shim b/package/quagga/quagga-shim
> new file mode 100644
> index 0000000..1687e7c
> --- /dev/null
> +++ b/package/quagga/quagga-shim
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +#Shim for systemd's quagga at .service

# Shim ...

> +
> +export QUAGGA_PATH="/usr/sbin"

 Why export?

> +
> +# Check if $1 is a valid quagga binary
> +case $1 in
> +  zebra)
> +    export BINARY="zebra"

 Why export?

 I think this check is really redundant. So what if someone calls
"quagga-shim tcpdump -i eth0" and it actually works? Is that a problem? It's not
as if this is setuid or anything.

 So I'd skip the entire case, and instead go straight to ...

[snip]
> +# Check the binary exists
> +if [ -e "$QUAGGA_PATH/$BINARY" ]; then

 And this check is redundant as well. Just call it, it will error out
automaticaly if the binary doesn't exist.

> +  shift # Remove the first argument (binary name)
> +  "$QUAGGA_PATH/$BINARY" "$@" # Run the daemon with the remaining arguments

 Is there any reason to keep the shim around? I.e., why not exec? Also, the
comment is quite redundant.


 In other words, I'd reduce the script to 5 lines:

#! /bin/sh
# Shim for systemd's quagga at .service
BINARY="$1"
shift
exec /usr/sbin/"$BINARY" "$@"



> +else
> +  echo "Binary \"$BINARY\" not found in $QUAGGA_PATH"
> +  exit 1
> +fi
> +
> diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk
> index 22e90ad..5d9c292 100644
> --- a/package/quagga/quagga.mk
> +++ b/package/quagga/quagga.mk
> @@ -75,6 +75,12 @@ endif
>  define QUAGGA_INSTALL_INIT_SYSTEMD
>  	$(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \
>  		$(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf
> +
> +	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
> +		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service

 In the commit log you mention zebra, here it's bandwidthd. /me is confused.

> +
> +	$(INSTALL) -D -m 755 package/quagga/quagga-shim \
> +		$(TARGET_DIR)/usr/sbin/quagga-shim
>  endef
>  
>  $(eval $(autotools-package))
> diff --git a/package/quagga/quagga at .service b/package/quagga/quagga at .service
> new file mode 100644
> index 0000000..3e98318
> --- /dev/null
> +++ b/package/quagga/quagga at .service
> @@ -0,0 +1,19 @@
> +[Unit]
> +Description=Quagga %i routing daemon
> +PartOf=quagga.service
> +ReloadPropagatedFrom=quagga.service
> +Wants=quagga at zebra.service
> +
> +[Service]
> +PrivateTmp=true
> +KillMode=mixed
> +Type=forking
> +EnvironmentFile=/etc/default/quagga-%i
> +ExecStart=/usr/sbin/quagga-shim %i --daemon $OPTS -f /etc/quagga/%i.conf -i /var/run/quagga/%i.pid

 systemd prefers non-forking daemons because then it has better control over it.
Since you give a --daemon option here, I assume that they are perfectly capable
of running as a simple service. Can you give that a try?


 Regards,
 Arnout

> +PIDFile=/var/run/quagga/%i.pid
> +KillSignal=SIGINT
> +Restart=on-failure
> +RestartSec=1
> +
> +[Install]
> +WantedBy=multi-user.target
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Arnout Vandecappelle]

 Two more things...

On 18-06-16 19:02, Nathaniel Roach wrote:
> +	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
> +		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service

 I believe it should actually be installed as quagga at .service, and then
symlinked from /etc.

 I'm not so sure of this, but wouldn't it actually make sense to install all
services that are selected in the config? Like

QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_ZEBRA) += zebra
QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_BGPD) += bgpd
...

	$(foreach service,$(QUAGGA_SERVICES_y),\
		ln -sf ../../../../usr/lib/systemd/system/quagga at .service \
		
$(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(service).service$(sep))

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Nathaniel Roach]

[snip]
>  So I'd skip the entire case, and instead go straight to ...
>
> [snip]
> > +# Check the binary exists
> > +if [ -e "$QUAGGA_PATH/$BINARY" ]; then
>
>  And this check is redundant as well. Just call it, it will error out
> automaticaly if the binary doesn't exist.
>
> > +  shift # Remove the first argument (binary name)
> > +  "$QUAGGA_PATH/$BINARY" "$@" # Run the daemon with the remaining
> arguments
>
>  Is there any reason to keep the shim around? I.e., why not exec?
> Also, the comment is quite redundant.
>
How about in the .service file simply ExecStart=/bin/sh -c "/usr/sbin/%i
..."?
>
>  In other words, I'd reduce the script to 5 lines:
>
> #! /bin/sh
> # Shim for systemd's quagga at .service
> BINARY="$1"
> shift
> exec /usr/sbin/"$BINARY" "$@"
>
>
>
> > +else
> > +  echo "Binary \"$BINARY\" not found in $QUAGGA_PATH"
> > +  exit 1
> > +fi
> > +
> > diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk
> > index 22e90ad..5d9c292 100644
> > --- a/package/quagga/quagga.mk
> > +++ b/package/quagga/quagga.mk
> > @@ -75,6 +75,12 @@ endif
> >  define QUAGGA_INSTALL_INIT_SYSTEMD
> >  	$(INSTALL) -D -m 644 package/quagga/quagga_tmpfiles.conf \
> >  		$(TARGET_DIR)/usr/lib/tmpfiles.d/quagga.conf
> > +
> > +	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
> > +		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service
>
>  In the commit log you mention zebra, here it's bandwidthd. /me is
> confused.
That was simply my mistake in copying and pasting that section of code
from my bandwidthd stuff without properly editing.
>
> > +
> > +	$(INSTALL) -D -m 755 package/quagga/quagga-shim \
> > +		$(TARGET_DIR)/usr/sbin/quagga-shim
> >  endef
> >
> >  $(eval $(autotools-package))
> > diff --git a/package/quagga/quagga at .service
b/package/quagga/quagga at .service
> > new file mode 100644
> > index 0000000..3e98318
> > --- /dev/null
> > +++ b/package/quagga/quagga at .service
> > @@ -0,0 +1,19 @@
> > +[Unit]
> > +Description=Quagga %i routing daemon
> > +PartOf=quagga.service
> > +ReloadPropagatedFrom=quagga.service
> > +Wants=quagga at zebra.service
> > +
> > +[Service]
> > +PrivateTmp=true
> > +KillMode=mixed
> > +Type=forking
> > +EnvironmentFile=/etc/default/quagga-%i
> > +ExecStart=/usr/sbin/quagga-shim %i --daemon $OPTS -f
/etc/quagga/%i.conf -i /var/run/quagga/%i.pid
>
>  systemd prefers non-forking daemons because then it has better
control over it.
> Since you give a --daemon option here, I assume that they are
perfectly capable
> of running as a simple service. Can you give that a try?

Sure, but would systemd's preference still apply with sh/bash in between
them?

>  Two more things...
> 
> On 18-06-16 19:02, Nathaniel Roach wrote:
>> +	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
>> +		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service
> 
>  I believe it should actually be installed as quagga at .service, and then
> symlinked from /etc.
> 
>  I'm not so sure of this, but wouldn't it actually make sense to install all
> services that are selected in the config? Like
> 
> QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_ZEBRA) += zebra
> QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_BGPD) += bgpd
> ...
> 
> 	$(foreach service,$(QUAGGA_SERVICES_y),\
> 		ln -sf ../../../../usr/lib/systemd/system/quagga at .service \
> 		
> $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(service).service$(sep))
You're right on how the service is used, except that the symlink would
be quagga at zebra.service.
I deliberately did not "install" the service file. If we are to do that,
we'll also need to create environment files and empty configuration
files for all of the selected daemons.
> 
>  Regards,
>  Arnout
> 

Thanks,

Nathaniel

[Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script

[Arnout Vandecappelle]

On 23-06-16 05:40, Nathaniel Roach wrote:
> [snip]
>>  So I'd skip the entire case, and instead go straight to ...
>>
>> [snip]
>>> +# Check the binary exists
>>> +if [ -e "$QUAGGA_PATH/$BINARY" ]; then
>>
>>  And this check is redundant as well. Just call it, it will error out
>> automaticaly if the binary doesn't exist.
>>
>>> +  shift # Remove the first argument (binary name)
>>> +  "$QUAGGA_PATH/$BINARY" "$@" # Run the daemon with the remaining
>> arguments
>>
>>  Is there any reason to keep the shim around? I.e., why not exec?
>> Also, the comment is quite redundant.
>>
> How about in the .service file simply ExecStart=/bin/sh -c "/usr/sbin/%i
> ..."?

 Yeah, even better. But add an exec, so

ExecStart=/bin/sh -c "exec /usr/sbin/%i ..."

Alternatively, you may be able to use

ExecStart=/usr/bin/env /usr/sbin/%i ...

BTW I would add a comment above it saying that systemd doesn't allow % to be
used in the command name, only in the arguments.

[snip]
>>> +ExecStart=/usr/sbin/quagga-shim %i --daemon $OPTS -f
> /etc/quagga/%i.conf -i /var/run/quagga/%i.pid
>>
>>  systemd prefers non-forking daemons because then it has better
> control over it.
>> Since you give a --daemon option here, I assume that they are
> perfectly capable
>> of running as a simple service. Can you give that a try?
> 
> Sure, but would systemd's preference still apply with sh/bash in between
> them?

 With exec or env, there is no intermediate (only temporarily).

> 
>>  Two more things...
>>
>> On 18-06-16 19:02, Nathaniel Roach wrote:
>>> +	$(INSTALL) -D -m 644 package/quagga/quagga at .service \
>>> +		$(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service
>>
>>  I believe it should actually be installed as quagga at .service, and then
>> symlinked from /etc.
>>
>>  I'm not so sure of this, but wouldn't it actually make sense to install all
>> services that are selected in the config? Like
>>
>> QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_ZEBRA) += zebra
>> QUAGGA_SERVICES_$(BR2_PACKAGE_QUAGGA_BGPD) += bgpd
>> ...
>>
>> 	$(foreach service,$(QUAGGA_SERVICES_y),\
>> 		ln -sf ../../../../usr/lib/systemd/system/quagga at .service \
>> 		
>> $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(service).service$(sep))
> You're right on how the service is used, except that the symlink would
> be quagga at zebra.service.

 Yes of course.

> I deliberately did not "install" the service file. If we are to do that,
> we'll also need to create environment files and empty configuration
> files for all of the selected daemons.

 OK. Perhaps mention that in the commit message.

 Regards,
 Arnout

>>
>>  Regards,
>>  Arnout
>>
> 
> Thanks,
> 
> Nathaniel
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF