Bug 1265 - contrib/unbound.service contains hardcoded path
contrib/unbound.service contains hardcoded path
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
x86_64 Linux
: P5 trivial
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-17 15:27 CEST by Petr Špaček
Modified: 2017-05-18 09:10 CEST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Petr Špaček 2017-05-17 15:27:02 CEST
In the source tree, file "contrib/unbound.service" contains hardcoded values:

[Service]
ExecReload=/bin/kill -HUP $MAINPID
ExecStart=/home/vagrant/unbound_systemd/unbound
ReadWritePaths=/etc/unbound /run

These should be replaced with values defined in autotools.


I guess that it will need Makefile.am file in contrib directory with something like:

%: %.in Makefile
        sed -e 's|@localstatedir[@]|$(localstatedir)|g' '$(srcdir)/$@.in' >$@

etc. for other directories. I did not test it.
Comment 1 Wouter Wijngaards 2017-05-17 15:30:50 CEST
Hi Petr,

It is fed by AC_CONFIG_FILES from configure, so all the symbols like @datarootdir@ and @sbindir@ and so on work.  I just don't know which values to use.  Which ones do you want?  If you can tell me what it should be, I can fix them to be autotool-filled in.

Also I have the suspicion that stuff like /etc/unbound and /run are distro specific.  But /home/user sounds like it could be improved :-)

Best regards, Wouter
Comment 2 Petr Špaček 2017-05-17 16:06:07 CEST
Thank you for reply. I think that values should correspond to values provided to configure by user. For example, Fedora build is executing configure with following values:

./configure --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-pythonmodule --with-pyunbound PYTHON=/usr/bin/python2 --with-libevent --with-pthreads --with-ssl --disable-rpath --disable-static --with-conf-file=/etc/unbound/unbound.conf --with-pidfile=/var/run/unbound/unbound.pid --enable-sha2 --disable-gost --enable-ecdsa --with-rootkey-file=/var/lib/unbound/root.key

@sbindir@ and should just work, but beware. It is autotools! :-) I think that the Makefile trick will be inevitable but you will see.
Comment 3 Wouter Wijngaards 2017-05-17 16:07:28 CEST
Hi Petr,

So, allright, thanks.  But what should be values for the service file be?  That is what I don't know.  Could you tell me what the contrib/unbound.service file would look like (with fake $thistypeofdir mentions in it) ?

Best regards, Wouter
Comment 4 Petr Špaček 2017-05-17 16:27:36 CEST
I apologize for not being clear.

IMHO the paths should be the same which are used by "make install" because, in the end "make install" is used by distos during package generation.

If we can make the service file with the values supplied to configure, distros will not need to maintain own service file and just use the one provided in contrib/.

Hopefully I explained it better this time. Have a nice day!
Comment 5 Wouter Wijngaards 2017-05-17 16:40:56 CEST
Hi Petr,

Sure, I can fill in those values (and I am getting that every time), but you are not getting the question that I am asking.  I don't know the correct contents of contrib/unbound.service.in and thus cannot also make it autotool filled in.  That is why that file is the way it is.

So I don't work with systemd every day (well, of course, my Fedora machine has it, but I don't fiddle around with it).  And I have no idea what reasonable values are, so right now these are the values that have been supplied by the previous contributor.

Thank you for keeping up with the confused conversation.  Autotool filled in is not my problem; reasonable text contents is.  Or, in an IETF analogy, send text (I can adjust it for the autotool stuff).

Best regards, Wouter
Comment 6 Wouter Wijngaards 2017-05-17 16:45:48 CEST
Hi Petr,

I'll just suggest some contents, and then you can correct me.

[Service]
ExecReload=@bindir@/kill -HUP $MAINPID
ExecStart=@sbindir@/unbound
ReadWritePaths=@sysconfdir@/unbound @localstatedir@ /run

Is this something that you would like?  Perhaps @sysconfdir@ without /unbound (so plain /etc?)

Best regards, Wouter
Comment 7 Wouter Wijngaards 2017-05-17 16:52:07 CEST
Hi Petr,

Ah, no I see, the file now has:
ExecReload=${exec_prefix}/bin/kill -HUP $MAINPID
ExecStart=${exec_prefix}/sbin/unbound
ReadWritePaths=${prefix}/etc ${prefix}/var /run

after processing :-)    Time to fix that.

Best regards, Wouter
Comment 8 Wouter Wijngaards 2017-05-17 17:04:43 CEST
Hi Petr,

Fixed it, these are the values.  Thanks for the report!  If you have other changes for the contrib/unbound.service file, just send 'em in.

ExecReload=@UNBOUND_BIN_DIR@/kill -HUP $MAINPID
ExecStart=@UNBOUND_SBIN_DIR@/unbound
ReadWritePaths=@UNBOUND_SYSCONF_DIR@ @UNBOUND_LOCALSTATE_DIR@ /run @UNBOUND_RUN_DIR@

with as output (for prefix of /usr/local)
ExecReload=/usr/local/bin/kill -HUP $MAINPID
ExecStart=/usr/local/sbin/unbound
ReadWritePaths=/usr/local/etc /usr/local/var /run /usr/local/etc/unbound

Best regards, Wouter
Comment 9 Petr Špaček 2017-05-17 17:07:55 CEST
The new values seem reasonable, thank you!
Comment 10 Robert Edmonds 2017-05-17 23:14:11 CEST
(In reply to Wouter Wijngaards from comment #8)
> Hi Petr,
> 
> Fixed it, these are the values.  Thanks for the report!  If you have other
> changes for the contrib/unbound.service file, just send 'em in.
> 
> ExecReload=@UNBOUND_BIN_DIR@/kill -HUP $MAINPID
> ExecStart=@UNBOUND_SBIN_DIR@/unbound
> ReadWritePaths=@UNBOUND_SYSCONF_DIR@ @UNBOUND_LOCALSTATE_DIR@ /run
> @UNBOUND_RUN_DIR@
> 
> with as output (for prefix of /usr/local)
> ExecReload=/usr/local/bin/kill -HUP $MAINPID
> ExecStart=/usr/local/sbin/unbound
> ReadWritePaths=/usr/local/etc /usr/local/var /run /usr/local/etc/unbound
> 
> Best regards, Wouter

Hi, Wouter and Petr:

/usr/local/bin/kill doesn't make any sense on Linux. The FHS requires /bin/kill to exist, and some distros symlink /bin to /usr/bin (e.g. https://fedoraproject.org/wiki/Features/UsrMove).

Using /bin/kill in unit files seems to be canonical, at least in the systemd documentation (https://www.freedesktop.org/software/systemd/man/systemd.service.html).
Comment 11 Wouter Wijngaards 2017-05-18 09:10:12 CEST
Hi Robert,

Thanks for the suggested fix.  Fixed to use /bin/kill and committed to the source repository.

Best regards, Wouter