Re: [Openvpn-devel] [PATCH 2/2] do not race on RuntimeDirectory
Just a very quick comment before I review and test the rest. On 24/01/17 15:39, Christian Hesse wrote: [...snip...] > @@ -20,6 +21,11 @@ if ENABLE_SYSTEMD > systemdunit_DATA = \ > openvpn-client@.service \ > openvpn-server@.service > +tmpfiles_DATA = \ > + tmpfiles-openvpn.conf > + > +install-data-hook: > + mv $(DESTDIR)$(tmpfilesdir)/tmpfiles-openvpn.conf > $(DESTDIR)$(tmpfilesdir)/openvpn.conf > endif Disregard my earlier comment about subdir. I think this is reasonable enough in this case. It was not as hacky as I feared. And this Makefile.am is so small and isolated it is good enough for me. If we regret it later on, it's an easy move into a tmpfiles.d subdir. -- kind regards, David Sommerseth OpenVPN Technologies, Inc signature.asc Description: OpenPGP digital signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] do not race on RuntimeDirectory
On 24/01/17 15:36, Christian Hesse wrote: > David Sommerseth on Fri, 2017/01/20 21:55: >> On 27/12/16 23:15, Christian Hesse wrote: >>> From: Christian Hesse >>> >>> Different unit instances create and destroy the same RuntimeDirectory. >>> This leads to running instances where the status file (and possibly >>> more runtime data) is no longer accessible. >>> >>> So do not handle this in unit files but provide a tmpfiles.d >>> configuration and let systemd-tmpfiles do the work. >>> Nobody will (unintentionally) delete the directories and its content. >>> As /run is volatile we do not have to care about cleanup. >>> >>> Signed-off-by: Christian Hesse >>> --- >>> configure.ac | 8 >>> distro/systemd/Makefile.am| 8 >>> distro/systemd/openvpn-cli...@.service.in | 2 -- >>> distro/systemd/openvpn-ser...@.service.in | 2 -- >>> distro/systemd/openvpn.conf | 2 ++ >>> 5 files changed, 18 insertions(+), 4 deletions(-) >>> create mode 100644 distro/systemd/openvpn.conf >>> >> >> [...snip...] >> >>> diff --git a/distro/systemd/Makefile.am b/distro/systemd/Makefile.am >>> index 53a88c9..1a6c974 100644 >>> --- a/distro/systemd/Makefile.am >>> +++ b/distro/systemd/Makefile.am >>> @@ -12,7 +12,12 @@ >>> $(AM_V_GEN)sed -e 's|\@sbindir\@|$(sbindir)|' \ >>> $< > $@.tmp && mv $@.tmp $@ >>> >>> +install-data-local: >>> + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-client >>> + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-server >> >> Hmm ... that doesn't make much sense, does it? >> >> $ mount | grep '/run ' >> tmpfs on /run type tmpfs (rw,nosuid,nodev,seclabel,mode=755) >> >> IIRC, upstream systemd recommends /run to be a ramdisk, as it should >> always be clean on freshly rebooted systems. And it probably saves a >> lot of SSD/flash storage write cycles, for those using that. > > Well, the idea is: Files in /usr/lib/tmpfiles.d/ are handled by > systemd-tmpfiles at boot time. We want these directories after installation > (without a reboot) to be present - so just create them. Yes, these reside on > tmpfs, but are created automatically after reboot. > > As package managers should run a hook if files in /usr/lib/tmpfiles.d/ change > (pacman does now) I am fine with removing this rule. > > People installing manually may still stumble on this... Ahh I see. I've not done any 'make install' on my systems for years; I build proper packages through mock/rpmbuild and install those. So much of that magic happens there. And I believe rpmbuild can complain loudly if it tries to own directories on a tmpfs. Okay, I'll ponder a little bit on this one and see what others do as well. You do have a valid point where users run 'make install' on a "production box". >> [...snip...] >> >>> @@ -21,6 +26,9 @@ systemdunitdir = $(systemdunitdir) >>> systemdunit_DATA = \ >>> openvpn-client@.service \ >>> openvpn-server@.service >>> +tmpfilesdir = $(tmpfilesdir) >> >> This conflicts with AC_SUBST([tmpfilesdir]) >> >> $ autoreconf -vi >> autoreconf: Entering directory `.' >> autoreconf: configure.ac: not using Gettext >> autoreconf: running: aclocal -I m4 >> autoreconf: configure.ac: tracing >> autoreconf: running: libtoolize --copy >> autoreconf: running: /usr/bin/autoconf >> autoreconf: running: /usr/bin/autoheader >> autoreconf: running: automake --add-missing --copy --no-force >> distro/systemd/Makefile.am:28: warning: tmpfilesdir was already defined >> in condition TRUE, which includes condition ENABLE_SYSTEMD ... >> configure.ac:1293: ... 'tmpfilesdir' previously defined here >> autoreconf: Leaving directory `.' >> $ rpm -q autoconf >> autoconf-2.69-11.el7.noarch >> >> Removing that tmpfilesdir declaration line in Makefile.am resolves this >> issue, and it still works as expected. > > Removed. > >> [...snip...] >> >>> diff --git a/distro/systemd/openvpn.conf b/distro/systemd/openvpn.conf >>> new file mode 100644 >>> index 000..57f20cd >>> --- /dev/null >>> +++ b/distro/systemd/openvpn.conf >>> @@ -0,0 +1,2 @@ >>> +d /run/openvpn-client 0710 root root - >>> +d /run/openvpn-server 0710 root root - >>> \ No newline at end of file >> >> This makes more sense though, as this will tell systemd to create these >> directories with the proper attributes. >> >> But I'm not too happy about the filename in our git repository. The >> destination file may very well be called openvpn.conf, as then it should >> reside in $libdir/tmpfiles.d/ ... but openvpn.conf causes quite a bit of >> ambiguity inside the openvpn source tree, and unaware users might more >> see this as a sample configuration for OpenVPN and be even more confused. >> >> I propose ... either rename this file to tmpfiles.d--openvpn.conf or >> move this openvpn.conf inside a tmpfiles.d/ subdirectory inside the >> ./distro/systemd/ directory. >> >> Otherwise, this looks very reasonable. > > Reworked this... Are we ok renaming the file in automake target > insta
Re: [Openvpn-devel] [PATCH 2/2] do not race on RuntimeDirectory
David Sommerseth on Fri, 2017/01/20 21:55:
> On 27/12/16 23:15, Christian Hesse wrote:
> > From: Christian Hesse
> >
> > Different unit instances create and destroy the same RuntimeDirectory.
> > This leads to running instances where the status file (and possibly
> > more runtime data) is no longer accessible.
> >
> > So do not handle this in unit files but provide a tmpfiles.d
> > configuration and let systemd-tmpfiles do the work.
> > Nobody will (unintentionally) delete the directories and its content.
> > As /run is volatile we do not have to care about cleanup.
> >
> > Signed-off-by: Christian Hesse
> > ---
> > configure.ac | 8
> > distro/systemd/Makefile.am| 8
> > distro/systemd/openvpn-cli...@.service.in | 2 --
> > distro/systemd/openvpn-ser...@.service.in | 2 --
> > distro/systemd/openvpn.conf | 2 ++
> > 5 files changed, 18 insertions(+), 4 deletions(-)
> > create mode 100644 distro/systemd/openvpn.conf
> >
>
> [...snip...]
>
> > diff --git a/distro/systemd/Makefile.am b/distro/systemd/Makefile.am
> > index 53a88c9..1a6c974 100644
> > --- a/distro/systemd/Makefile.am
> > +++ b/distro/systemd/Makefile.am
> > @@ -12,7 +12,12 @@
> > $(AM_V_GEN)sed -e 's|\@sbindir\@|$(sbindir)|' \
> > $< > $@.tmp && mv $@.tmp $@
> >
> > +install-data-local:
> > + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-client
> > + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-server
>
> Hmm ... that doesn't make much sense, does it?
>
> $ mount | grep '/run '
> tmpfs on /run type tmpfs (rw,nosuid,nodev,seclabel,mode=755)
>
> IIRC, upstream systemd recommends /run to be a ramdisk, as it should
> always be clean on freshly rebooted systems. And it probably saves a
> lot of SSD/flash storage write cycles, for those using that.
Well, the idea is: Files in /usr/lib/tmpfiles.d/ are handled by
systemd-tmpfiles at boot time. We want these directories after installation
(without a reboot) to be present - so just create them. Yes, these reside on
tmpfs, but are created automatically after reboot.
As package managers should run a hook if files in /usr/lib/tmpfiles.d/ change
(pacman does now) I am fine with removing this rule.
People installing manually may still stumble on this...
> [...snip...]
>
> > @@ -21,6 +26,9 @@ systemdunitdir = $(systemdunitdir)
> > systemdunit_DATA = \
> > openvpn-client@.service \
> > openvpn-server@.service
> > +tmpfilesdir = $(tmpfilesdir)
>
> This conflicts with AC_SUBST([tmpfilesdir])
>
> $ autoreconf -vi
> autoreconf: Entering directory `.'
> autoreconf: configure.ac: not using Gettext
> autoreconf: running: aclocal -I m4
> autoreconf: configure.ac: tracing
> autoreconf: running: libtoolize --copy
> autoreconf: running: /usr/bin/autoconf
> autoreconf: running: /usr/bin/autoheader
> autoreconf: running: automake --add-missing --copy --no-force
> distro/systemd/Makefile.am:28: warning: tmpfilesdir was already defined
> in condition TRUE, which includes condition ENABLE_SYSTEMD ...
> configure.ac:1293: ... 'tmpfilesdir' previously defined here
> autoreconf: Leaving directory `.'
> $ rpm -q autoconf
> autoconf-2.69-11.el7.noarch
>
> Removing that tmpfilesdir declaration line in Makefile.am resolves this
> issue, and it still works as expected.
Removed.
> [...snip...]
>
> > diff --git a/distro/systemd/openvpn.conf b/distro/systemd/openvpn.conf
> > new file mode 100644
> > index 000..57f20cd
> > --- /dev/null
> > +++ b/distro/systemd/openvpn.conf
> > @@ -0,0 +1,2 @@
> > +d /run/openvpn-client 0710 root root -
> > +d /run/openvpn-server 0710 root root -
> > \ No newline at end of file
>
> This makes more sense though, as this will tell systemd to create these
> directories with the proper attributes.
>
> But I'm not too happy about the filename in our git repository. The
> destination file may very well be called openvpn.conf, as then it should
> reside in $libdir/tmpfiles.d/ ... but openvpn.conf causes quite a bit of
> ambiguity inside the openvpn source tree, and unaware users might more
> see this as a sample configuration for OpenVPN and be even more confused.
>
> I propose ... either rename this file to tmpfiles.d--openvpn.conf or
> move this openvpn.conf inside a tmpfiles.d/ subdirectory inside the
> ./distro/systemd/ directory.
>
> Otherwise, this looks very reasonable.
Reworked this... Are we ok renaming the file in automake target
install-data-hook? Could not come up with a better solution.
I will send an updated patch soon.
--
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}
pgpilcxLfwWWR.pgp
Description: OpenPGP digital signature
--
Check out the vibrant tech community on one of the world's most
enga
Re: [Openvpn-devel] [PATCH 2/2] do not race on RuntimeDirectory
On 27/12/16 23:15, Christian Hesse wrote: > From: Christian Hesse > > Different unit instances create and destroy the same RuntimeDirectory. > This leads to running instances where the status file (and possibly > more runtime data) is no longer accessible. > > So do not handle this in unit files but provide a tmpfiles.d > configuration and let systemd-tmpfiles do the work. > Nobody will (unintentionally) delete the directories and its content. > As /run is volatile we do not have to care about cleanup. > > Signed-off-by: Christian Hesse > --- > configure.ac | 8 > distro/systemd/Makefile.am| 8 > distro/systemd/openvpn-cli...@.service.in | 2 -- > distro/systemd/openvpn-ser...@.service.in | 2 -- > distro/systemd/openvpn.conf | 2 ++ > 5 files changed, 18 insertions(+), 4 deletions(-) > create mode 100644 distro/systemd/openvpn.conf > [...snip...] > diff --git a/distro/systemd/Makefile.am b/distro/systemd/Makefile.am > index 53a88c9..1a6c974 100644 > --- a/distro/systemd/Makefile.am > +++ b/distro/systemd/Makefile.am > @@ -12,7 +12,12 @@ > $(AM_V_GEN)sed -e 's|\@sbindir\@|$(sbindir)|' \ > $< > $@.tmp && mv $@.tmp $@ > > +install-data-local: > + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-client > + $(INSTALL) -d -m0710 $(DESTDIR)/run/openvpn-server Hmm ... that doesn't make much sense, does it? $ mount | grep '/run ' tmpfs on /run type tmpfs (rw,nosuid,nodev,seclabel,mode=755) IIRC, upstream systemd recommends /run to be a ramdisk, as it should always be clean on freshly rebooted systems. And it probably saves a lot of SSD/flash storage write cycles, for those using that. [...snip...] > @@ -21,6 +26,9 @@ systemdunitdir = $(systemdunitdir) > systemdunit_DATA = \ > openvpn-client@.service \ > openvpn-server@.service > +tmpfilesdir = $(tmpfilesdir) This conflicts with AC_SUBST([tmpfilesdir]) $ autoreconf -vi autoreconf: Entering directory `.' autoreconf: configure.ac: not using Gettext autoreconf: running: aclocal -I m4 autoreconf: configure.ac: tracing autoreconf: running: libtoolize --copy autoreconf: running: /usr/bin/autoconf autoreconf: running: /usr/bin/autoheader autoreconf: running: automake --add-missing --copy --no-force distro/systemd/Makefile.am:28: warning: tmpfilesdir was already defined in condition TRUE, which includes condition ENABLE_SYSTEMD ... configure.ac:1293: ... 'tmpfilesdir' previously defined here autoreconf: Leaving directory `.' $ rpm -q autoconf autoconf-2.69-11.el7.noarch Removing that tmpfilesdir declaration line in Makefile.am resolves this issue, and it still works as expected. [...snip...] > diff --git a/distro/systemd/openvpn.conf b/distro/systemd/openvpn.conf > new file mode 100644 > index 000..57f20cd > --- /dev/null > +++ b/distro/systemd/openvpn.conf > @@ -0,0 +1,2 @@ > +d /run/openvpn-client 0710 root root - > +d /run/openvpn-server 0710 root root - > \ No newline at end of file This makes more sense though, as this will tell systemd to create these directories with the proper attributes. But I'm not too happy about the filename in our git repository. The destination file may very well be called openvpn.conf, as then it should reside in $libdir/tmpfiles.d/ ... but openvpn.conf causes quite a bit of ambiguity inside the openvpn source tree, and unaware users might more see this as a sample configuration for OpenVPN and be even more confused. I propose ... either rename this file to tmpfiles.d--openvpn.conf or move this openvpn.conf inside a tmpfiles.d/ subdirectory inside the ./distro/systemd/ directory. Otherwise, this looks very reasonable. -- kind regards, David Sommerseth OpenVPN Technologies, Inc signature.asc Description: OpenPGP digital signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
