| Message ID | 1466269320-23938-1-git-send-email-nroach44@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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@.service | 19 ++++++++++++++++ > 3 files changed, 76 insertions(+) > create mode 100644 package/quagga/quagga-shim > create mode 100644 package/quagga/quagga@.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@.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@.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@.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@.service b/package/quagga/quagga@.service > new file mode 100644 > index 0000000..3e98318 > --- /dev/null > +++ b/package/quagga/quagga@.service > @@ -0,0 +1,19 @@ > +[Unit] > +Description=Quagga %i routing daemon > +PartOf=quagga.service > +ReloadPropagatedFrom=quagga.service > +Wants=quagga@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 >
Two more things... On 18-06-16 19:02, Nathaniel Roach wrote: > + $(INSTALL) -D -m 644 package/quagga/quagga@.service \ > + $(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service I believe it should actually be installed as quagga@.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@.service \ $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(service).service$(sep)) Regards, Arnout
[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@.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@.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@.service b/package/quagga/quagga@.service > > new file mode 100644 > > index 0000000..3e98318 > > --- /dev/null > > +++ b/package/quagga/quagga@.service > > @@ -0,0 +1,19 @@ > > +[Unit] > > +Description=Quagga %i routing daemon > > +PartOf=quagga.service > > +ReloadPropagatedFrom=quagga.service > > +Wants=quagga@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@.service \ >> + $(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service > > I believe it should actually be installed as quagga@.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@.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@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
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@.service \ >>> + $(TARGET_DIR)/usr/lib/systemd/system/bandwidthd.service >> >> I believe it should actually be installed as quagga@.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@.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@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 >
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@.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@.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@.service b/package/quagga/quagga@.service new file mode 100644 index 0000000..3e98318 --- /dev/null +++ b/package/quagga/quagga@.service @@ -0,0 +1,19 @@ +[Unit] +Description=Quagga %i routing daemon +PartOf=quagga.service +ReloadPropagatedFrom=quagga.service +Wants=quagga@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
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@.service | 19 ++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 package/quagga/quagga-shim create mode 100644 package/quagga/quagga@.service