[Buildroot] [PATCH v3] package/quagga: Add systemd.service file

[Nathaniel Roach <nroach44@gmail.com>]

On 03/07/16 20:59, Maxime Hadjinlian wrote:
> Hi Nathaniel,
>
> On Sun, Jul 3, 2016 at 12:54 PM, Nathaniel Roach <nroach44@gmail.com> wrote:
>> Hi Maxime,
>>
>>
>>
>> On 03/07/16 18:42, Maxime Hadjinlian wrote:
>>> Hi Nathaniel, all
>>>
>>> On Sun, Jul 3, 2016 at 10:18 AM, Nathaniel Roach <nroach44@gmail.com>
>>> wrote:
>>>> 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.
>>>>
>>>> /usr/bin/env is needed as systemd doesn't allow the instance
>>>> variable (%i) in the executable path.
>>>>
>>>> We don't enable these services by default as this would require
>>>> creating configuration and /etc/default files. (And is easily
>>>> achieved with an FS overlay)
>>>>
>>>> Signed-off-by: Nathaniel Roach <nroach44@gmail.com>
>>>> ---
>>>> Changes v2 -> v3:
>>>>    - Remove invalid references to quagga.service (Arnout)
>>>>    - Check if the binary is executable before trying to start it
>>>>      (Arnout)
>>>>    - Remove PID file arguments and options (Arnout)
>>>>    - Add reload capability as the daemons do support it
>>>>
>>>> Changes v1 -> v2:
>>>>    (As suggested by Arnout Vandecappelle)
>>>>    - Completely remove shim and use /usr/bin/env instead
>>>>    - Don't tell quagga to fork as systemd prefers it
>>>>    - Add comment to .service file about /usr/bin/env
>>>>    - Explain not enabling the service on build in patch
>>>> ---
>>>>    package/quagga/quagga.mk       |  2 ++
>>>>    package/quagga/quagga at .service | 20 ++++++++++++++++++++
>>>>    2 files changed, 22 insertions(+)
>>>>    create mode 100644 package/quagga/quagga at .service
>>>>
>>>> diff --git a/package/quagga/quagga.mk b/package/quagga/quagga.mk
>>>> index 22e90ad..1bbc72d 100644
>>>> --- a/package/quagga/quagga.mk
>>>> +++ b/package/quagga/quagga.mk
>>>> @@ -75,6 +75,8 @@ 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/quagga at .service
>>>>    endef
>>>>
>>>>    $(eval $(autotools-package))
>>>> diff --git a/package/quagga/quagga at .service
>>>> b/package/quagga/quagga at .service
>>>> new file mode 100644
>>>> index 0000000..16acc30
>>>> --- /dev/null
>>>> +++ b/package/quagga/quagga at .service
>>>> @@ -0,0 +1,20 @@
>>>> +[Unit]
>>>> +Description=Quagga %i routing daemon
>>>> +ConditionFileIsExecutable=/usr/sbin/%i
>>>> +Wants=quagga at zebra.service
>>> Just a though, I read about why you added the Wants, maybe a comment
>>> would be nice here ?
>>>> +
>>>> +[Service]
>>>> +Type=simple
>>> Since you have removed the '--daemon' option, maybe you should revert
>>> this to "forking" ?
>> I might have it backwards, but it would need "forking" if --daemon was
>> specified, otherwise "systemctl start quagga at ospfd" would hang.
> You are absolutely right, I am the one that got confused. I missed the
> '-d' on the ExecStart line in the services in the quagga sources.
>>>> +EnvironmentFile=/etc/default/quagga-%i
>>>> +PrivateTmp=true
>>>> +# Systemd doesn't like having %i in the executable path.
>>>> +ExecStart=/usr/bin/env /usr/sbin/%i $OPTS -f /etc/quagga/%i.conf
>>>> +ExecReload=/bin/kill -HUP $MAINPID
>>>> +KillMode=mixed
>>>> +KillSignal=SIGINT
>>> If I understand correctly, what you want to do is send a SIGINT to the
>>> main processes and SIGKILL to the other ? From the services file I can
>>> find in major distro, they don't specify it so it's left to the
>>> default SIGTERM, why did you need to change that ? (Maybe you already
>>> explained in your previous mail and I missed, sorry if that's the
>>> case).
>> KillMode=Mixed is an artefact left over from the OpenVPN .service file I
>> used as a reference. I (again) misread the systemd docs and left it in as it
>> seemed more appropriate than the default if any child processes are started.
>> This can be removed on commit, or I'll fix it if there's any other things
>> that need to be changed.
> Thanks for the explanation.
>>>> +Restart=on-failure
>>>> +RestartSec=1
>>> Why ?
>> I've had ospfd crash during configuration and general usage, this is
>> undesirable as it's responsible for keeping a large amount of routes in my
>> router's tables.
> I was thinking about the RestartSec delay, I agree on the "on-failure".
Oh, derp. I didn't pick 1 second for any reason other than it's 
quickest. If there's a good argument for longer it can be set longer.
>>>> +
>>>> +[Install]
>>>> +WantedBy=multi-user.target
>>>> +
>>>> --
>>>> 2.8.1
>>>>
>>>> _______________________________________________
>>>> buildroot mailing list
>>>> buildroot at busybox.net
>>>> http://lists.busybox.net/mailman/listinfo/buildroot
>>