* [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script @ 2016-06-18 17:02 Nathaniel Roach 2016-06-22 21:49 ` Arnout Vandecappelle 2016-06-22 21:58 ` Arnout Vandecappelle 0 siblings, 2 replies; 5+ messages in thread From: Nathaniel Roach @ 2016-06-18 17:02 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script 2016-06-18 17:02 [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script Nathaniel Roach @ 2016-06-22 21:49 ` Arnout Vandecappelle 2016-06-22 21:58 ` Arnout Vandecappelle 1 sibling, 0 replies; 5+ messages in thread From: Arnout Vandecappelle @ 2016-06-22 21:49 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script 2016-06-18 17:02 [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script Nathaniel Roach 2016-06-22 21:49 ` Arnout Vandecappelle @ 2016-06-22 21:58 ` Arnout Vandecappelle 2016-06-23 3:40 ` Nathaniel Roach 1 sibling, 1 reply; 5+ messages in thread From: Arnout Vandecappelle @ 2016-06-22 21:58 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script 2016-06-22 21:58 ` Arnout Vandecappelle @ 2016-06-23 3:40 ` Nathaniel Roach 2016-06-23 21:29 ` Arnout Vandecappelle 0 siblings, 1 reply; 5+ messages in thread From: Nathaniel Roach @ 2016-06-23 3:40 UTC (permalink / raw) To: buildroot [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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script 2016-06-23 3:40 ` Nathaniel Roach @ 2016-06-23 21:29 ` Arnout Vandecappelle 0 siblings, 0 replies; 5+ messages in thread From: Arnout Vandecappelle @ 2016-06-23 21:29 UTC (permalink / raw) To: buildroot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-23 21:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-18 17:02 [Buildroot] [PATCH 1/1] package/quagga: Add systemd.service file and helper script Nathaniel Roach 2016-06-22 21:49 ` Arnout Vandecappelle 2016-06-22 21:58 ` Arnout Vandecappelle 2016-06-23 3:40 ` Nathaniel Roach 2016-06-23 21:29 ` Arnout Vandecappelle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox