On Sun, Jun 25, 2023 at 11:06:14AM +0200, Francesco P. Lovergine wrote:
> On Sat, Jun 24, 2023 at 05:45:33PM +0100, Jonathan Wiltshire wrote:
> > > diff -Nru proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.prerm
> > > proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.prerm
> > > --- proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.prerm 1970-01-01
> > > 01:00:00.000000000 +0100
> > > +++ proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.prerm 2023-06-22
> > > 11:13:30.000000000 +0200
> > > @@ -0,0 +1,11 @@
> > > +#!/bin/sh
> > > +
> > > +set -e
> > > +
> > > +if [ -z "${DPKG_ROOT:-}" ] && [ "$1" = remove ] && [ -d
> > > /run/systemd/system ] ;
> > > +then
> > > + deb-systemd-invoke stop 'proftpd.service' >/dev/null || true
> > > + deb-systemd-invoke stop 'proftpd.socket' >/dev/null || true
> > > +fi
> >
> > This gives rise to a race condition where the socket starts the service
> > again before the socket is stopped.
> >
>
> Well, this is exactly what debhelper does in current prerm in bookworm.
> Eventually, it could be unified in `deb-systemd-invoke stop 'proftpd.socket'
> 'proftpd.service' || true` like other packages do.
> I'm not sure if this is what you intend, but if that risks a race condition
> it would applies to
> a lots of other packages.
Ok, I didn't look at the debhelper behaviour. It's not a big risk.
> > > diff -Nru proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.proftpd-run.service
> > > proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.proftpd-run.service
> > > --- proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.proftpd-run.service
> > > 1970-01-01 01:00:00.000000000 +0100
> > > +++ proftpd-dfsg-1.3.8+dfsg/debian/proftpd-core.proftpd-run.service
> > > 2023-06-22 11:12:42.000000000 +0200
> > > @@ -0,0 +1,14 @@
> > > +[Unit]
> > > +Description=ProFTPD FTP Server in standalone/socket mode
> > > +Documentation=man:proftpd(8)
> > > +OnFailure=proftpd.socket
> > > +OnSuccess=proftpd.service
> > > +
> > > +[Service]
> > > +Type=oneshot
> > > +Environment=CONFIG_FILE=/etc/proftpd/proftpd.conf
> > > +EnvironmentFile=-/etc/default/proftpd
> > > +ExecStart=/usr/bin/grep -iqE
> > > '^[[:space:]]*ServerType[[:space:]]+standalone$' $CONFIG_FILE
> >
> > Maybe I missed something important, but this seems a very odd way of doing
> > things. Do you really set up a dummy service unit which is expected to fail
> > in standalone mode, and therefore starts the socket instead?
> >
> > Why not use an ExecStartPre= or ExecCondition= in your normal units to
> > prevent starting when in inetd mode?
> >
>
> Unfortunately, Exec* directives can only be used in .service units.
This statement is at odds with the documentation for systemd.socket(5).
> That's
> the reason to enable an external oneshot .service unit to start
> alternatively one of the two other units. Ideally one day or another such
> features could
> be available also in other type of units (there is an issue open since
> 2019). Incidentally, it is possible to add a ConditionPathExists and a
> something like /etc/proftpd/proftpd_not_to_be_run (which is the trick used
> in sshd) but would be completely Debian specific and out of the usual
> workflow to manage inetd/standalone modes in proftpd. So, I'm not that keen
> on
> this kind of trick.
I don't think a ConditionPathExists hack is necessary here. Yes, in the
standalone case you will end up with the socket unit failing, and the local
admin will have to disable that if it annoys them, but any competent
administrator should be able to figure that out with the help of
NEWS.Debian.
--
Jonathan Wiltshire j...@debian.org
Debian Developer http://people.debian.org/~jmw
4096R: 0xD3524C51 / 0A55 B7C5 1223 3942 86EC 74C3 5394 479D D352 4C51
ed25519/0x196418AAEB74C8A1: CA619D65A72A7BADFC96D280196418AAEB74C8A1
