Bug#562506: init scripts should not use set -e
Russ Allbery
Version: 3.8.3.0
Severity: minor
Currently, Debian Policy makes a general statement that all Bourne shell
scripts should start with set -e and does not (so far as I can see) make
an exception for init scripts. I've seen several init scripts use set -e,
which is usually a bad idea. It assumes that the shell function libraries
used for status reporting are "set -e"-clean, which they may not be, and
it causes the init scripts to exit in non-obvious ways and produce lots of
debugging headache.
I think there's already a consensus that set -e is the wrong approach for
init scripts and instead the exit status of key commands should be checked
instead. I think Policy should reflect that consensus somewhere in the
section on init scripts.
-- System Information:
Debian Release: squeeze/sid
APT prefers testing
APT policy: (990, 'testing'), (500, 'unstable'), (1, 'experimental')
Architecture: i386 (i686)
Kernel: Linux 2.6.30-2-686-bigmem (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
debian-policy depends on no packages.
debian-policy recommends no packages.
Versions of packages debian-policy suggests:
ii doc-base 0.9.5 utilities to manage online documen
-- no debconf information
--
To UNSUBSCRIBE, email to debian-bugs-...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listm...@lists.debian.org
Guillem Jover
On Thu, 2009-12-24 at 16:07:48 -0800, Russ Allbery wrote:
> Package: debian-policy
> Version: 3.8.3.0
> Severity: minor
> Currently, Debian Policy makes a general statement that all Bourne shell
> scripts should start with set -e and does not (so far as I can see) make
> an exception for init scripts. I've seen several init scripts use set -e,
> which is usually a bad idea. It assumes that the shell function libraries
> used for status reporting are "set -e"-clean, which they may not be, and
> it causes the init scripts to exit in non-obvious ways and produce lots of
> debugging headache.
In particular calls like “log_end_msg $?” after start-stop-daemon are
the obvious buggy ofenders. Or it makes the script do some interesting
gymnastics to retrieve the return code to be able to pass it on to
log_end_msg for example.
> I think there's already a consensus that set -e is the wrong approach for
> init scripts and instead the exit status of key commands should be checked
> instead. I think Policy should reflect that consensus somewhere in the
> section on init scripts.
Yes, please.
regards,
guillem
Steve Langasek
> Currently, Debian Policy makes a general statement that all Bourne shell
> scripts should start with set -e and does not (so far as I can see) make
> an exception for init scripts. I've seen several init scripts use set -e,
> which is usually a bad idea. It assumes that the shell function libraries
> used for status reporting are "set -e"-clean, which they may not be, and
> it causes the init scripts to exit in non-obvious ways and produce lots of
> debugging headache.
> I think there's already a consensus that set -e is the wrong approach for
> init scripts and instead the exit status of key commands should be checked
> instead. I think Policy should reflect that consensus somewhere in the
> section on init scripts.
I don't think that 'set -e' is wrong for init scripts, so much as the LSB
init script interfaces' requirement of 'set +e' is an unfortunate design
flaw. Still, as many init scripts are using /lib/lsb/init-functions
nowadays, I think it's worth calling this out in Policy as an exception.
We should explicitly qualify this in terms of /lib/lsb/init-functions being
an *optional* library that init scripts *may* use.
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
slan...@ubuntu.com vor...@debian.org
Russ Allbery
> I don't think that 'set -e' is wrong for init scripts, so much as the
> LSB init script interfaces' requirement of 'set +e' is an unfortunate
> design flaw. Still, as many init scripts are using
> /lib/lsb/init-functions nowadays, I think it's worth calling this out in
> Policy as an exception.
> We should explicitly qualify this in terms of /lib/lsb/init-functions
> being an *optional* library that init scripts *may* use.
That seems reasonable, although I think we should also point out the
problems with using set -e when starting a daemon, namely that you need to
be sure to wrap the start-stop-daemon invocation in a conditional so that
you can properly report errors, rather than just letting the init script
die.
--
Russ Allbery (r...@debian.org) <http://www.eyrie.org/~eagle/>
Vincent Danjean
> Steve Langasek<vor...@debian.org> writes:
>
>> I don't think that 'set -e' is wrong for init scripts, so much as the
>> LSB init script interfaces' requirement of 'set +e' is an unfortunate
>> design flaw. Still, as many init scripts are using
>> /lib/lsb/init-functions nowadays, I think it's worth calling this out in
>> Policy as an exception.
>
>> We should explicitly qualify this in terms of /lib/lsb/init-functions
>> being an *optional* library that init scripts *may* use.
>
> That seems reasonable, although I think we should also point out the
> problems with using set -e when starting a daemon, namely that you need to
> be sure to wrap the start-stop-daemon invocation in a conditional so that
> you can properly report errors, rather than just letting the init script
> die.
RET=0
start-stop-daemon .... || RET=$?
log_end_msg $RET
This is a construction I often use in my scripts that avoid a "if then fi"
Regards,
Vincent
--
Vincent Danjean GPG key ID 0x9D025E87 vdan...@debian.org
GPG key fingerprint: FC95 08A6 854D DB48 4B9A 8A94 0BF7 7867 9D02 5E87
Unofficial packages: http://moais.imag.fr/membres/vincent.danjean/deb.html
APT repo: deb http://perso.debian.org/~vdanjean/debian unstable main
Bastian Blank
> That seems reasonable, although I think we should also point out the
> problems with using set -e when starting a daemon, namely that you need to
> be sure to wrap the start-stop-daemon invocation in a conditional so that
> you can properly report errors, rather than just letting the init script
> die.
set -e produces problems like this: #546743.
Bastian
--
He's dead, Jim.
-- McCoy, "The Devil in the Dark", stardate 3196.1
martin f krafft
> set -e produces problems like this: #546743.
lsb-base should not get away with "because we write shitty shell
code and don't do proper error handling, we expect everyone else to
do alike". If set -e is causing problems, remember that we're
talking about symptoms.
I am reminded of http://madduck.net/blog/2006.09.22:why-i-don-t-work-on-ubuntu/
--
.''`. martin f. krafft <madduck@d.o> Related projects:
: :' : proud Debian developer http://debiansystem.info
`. `'` http://people.debian.org/~madduck http://vcs-pkg.org
`- Debian - when you have better things to do than fixing systems
Russ Allbery
> also sprach Bastian Blank <wa...@debian.org> [2010.02.27.1320 +0100]:
>> set -e produces problems like this: #546743.
> lsb-base should not get away with "because we write shitty shell code
> and don't do proper error handling, we expect everyone else to do
> alike". If set -e is causing problems, remember that we're talking about
> symptoms.
I think lsb-base should be fixed, but I also think set -e in an init
script is a bad idea. I would argue that specifically because running
commands that fail is a normal and expected init script operation, unlike
nearly every other shell script, so set -e just makes it harder to write a
script that functions correctly according to Policy. It also makes the
init script fragile in ways that are painfully hard to debug when things
like the LSB functions don't account for set -e.
I've personally run into three or four serious bugs in packages because of
set -e, and I've never seen a case where having set -e prevented or
diagnosed a problem that would have otherwise gone unnoticed.
--
Russ Allbery (r...@debian.org) <http://www.eyrie.org/~eagle/>
--
martin f krafft
> I think lsb-base should be fixed, but I also think set -e in an init
> script is a bad idea. I would argue that specifically because running
> commands that fail is a normal and expected init script operation, unlike
> nearly every other shell script, so set -e just makes it harder to write a
> script that functions correctly according to Policy. It also makes the
> init script fragile in ways that are painfully hard to debug when things
> like the LSB functions don't account for set -e.
>
> I've personally run into three or four serious bugs in packages because of
> set -e, and I've never seen a case where having set -e prevented or
> diagnosed a problem that would have otherwise gone unnoticed.
I still think set -e is a good idea, but I realise it boils down to
preference. If your experience is representative, then it's probably
better to advocate not setting set -e in init scripts.
What about maintainer scripts?
--
.''`. martin f. krafft <madduck@d.o> Related projects:
: :' : proud Debian developer http://debiansystem.info
`. `'` http://people.debian.org/~madduck http://vcs-pkg.org
`- Debian - when you have better things to do than fixing systems
"president thieu says he'll quit if he doesn't get more than 50% of
the vote. in a democracy, that's not called quitting."
-- the washington post
Russ Allbery
> I still think set -e is a good idea, but I realise it boils down to
> preference. If your experience is representative, then it's probably
> better to advocate not setting set -e in init scripts.
> What about maintainer scripts?
It's almost always correct, in a maintainer script, for the script to
abort if anything it attempted to do failed. If the maintainer hadn't
anticipated that failure, having the script continue anyway risks in some
cases severe data corruption. I think it's therefore much safer to have
set -e in maintainer scripts and support using it there.
With init scripts, however, since Debian Policy requires that starting a
daemon that's already running or stopping a daemon that isn't running
succeed, set -e requires special checking for those cases. If the
maintainer does that when writing the init script, great, but it seems to
be a very frequent source of bugs. (I just ran into another case of that
the other day and need to file a bug about it.)
Init scripts also produce output normally, but if echo fails because
something is wrong with the console, having the init script silently abort
is very tricky to debug. I suppose that's also the same with maintainer
scripts, but it's much less common for maintainer scripts to echo things
to the console.
Goswin von Brederlow
> martin f krafft <mad...@debian.org> writes:
>
>> I still think set -e is a good idea, but I realise it boils down to
>> preference. If your experience is representative, then it's probably
>> better to advocate not setting set -e in init scripts.
>
>> What about maintainer scripts?
>
> It's almost always correct, in a maintainer script, for the script to
> abort if anything it attempted to do failed. If the maintainer hadn't
> anticipated that failure, having the script continue anyway risks in some
> cases severe data corruption. I think it's therefore much safer to have
> set -e in maintainer scripts and support using it there.
>
> With init scripts, however, since Debian Policy requires that starting a
> daemon that's already running or stopping a daemon that isn't running
> succeed, set -e requires special checking for those cases. If the
> maintainer does that when writing the init script, great, but it seems to
> be a very frequent source of bugs. (I just ran into another case of that
> the other day and need to file a bug about it.)
>
> Init scripts also produce output normally, but if echo fails because
> something is wrong with the console, having the init script silently abort
> is very tricky to debug. I suppose that's also the same with maintainer
> scripts, but it's much less common for maintainer scripts to echo things
> to the console.
Plus set -e gives a false sense of security. As soon as you have
functions in conditionals or compound statements it does no longer work.
E.g.
set -e
start() {
check this
do that
do start the daemon
}
if start; then
SUCCESS
else
FAILED
fi
You would think that the script aborts if anything in start() fails but
no such luck.
Also init scripts should not just abort. What about those nice red
FAILED messages during boot? :)
MfG
Goswin
Russ Allbery
> Currently, Debian Policy makes a general statement that all Bourne shell
> scripts should start with set -e and does not (so far as I can see) make
> an exception for init scripts. I've seen several init scripts use set
> -e, which is usually a bad idea. It assumes that the shell function
> libraries used for status reporting are "set -e"-clean, which they may
> not be, and it causes the init scripts to exit in non-obvious ways and
> produce lots of debugging headache.
> I think there's already a consensus that set -e is the wrong approach
> for init scripts and instead the exit status of key commands should be
> checked instead. I think Policy should reflect that consensus somewhere
> in the section on init scripts.
Here is proposed wording, which hopefully reflects the subsequent
discussion. I'm looking for seconds.
diff --git a/policy.sgml b/policy.sgml
index d16df70..1871d4c 100644
--- a/policy.sgml
+++ b/policy.sgml
@@ -6011,7 +6011,7 @@ rmdir /usr/local/share/emacs 2>/dev/null || true
</p>
</sect1>
- <sect1>
+ <sect1 id="writing-init">
<heading>Writing the scripts</heading>
<p>
@@ -6062,6 +6062,23 @@ rmdir /usr/local/share/emacs 2>/dev/null || true
</p>
<p>
+ Be careful of using <tt>set -e</tt> in <file>init.d</file>
+ scripts. Writing correct <file>init.d</file> scripts requires
+ accepting various error exit statuses when daemons are already
+ running or already stopped without aborting
+ the <file>init.d</file> script, and common <file>init.d</file>
+ function libraries are not safe to call with <tt>set -e</tt>
+ in effect<footnote>
+ <tt>/lib/lsb/init-functions</tt>, which assists in writing
+ LSB-compliant init scripts, may fail if <tt>set -e</tt> is
+ in effect and echoing status messages to the console fails,
+ for example.
+ </footnote>. For <tt>init.d</tt> scripts, it's often easier
+ to not use <tt>set -e</tt> and instead check the result of
+ each command separately.
+ </p>
+
+ <p>
If a service reloads its configuration automatically (as
in the case of <prgn>cron</prgn>, for example), the
<tt>reload</tt> option of the <file>init.d</file> script
@@ -7158,13 +7175,19 @@ strip --strip-unneeded <var>your-lib</var>
language currently used to implement it.
</p>
<p>
- Shell scripts (<prgn>sh</prgn> and <prgn>bash</prgn>)
- should almost certainly start with <tt>set -e</tt> so that
- errors are detected. Every script should use
- <tt>set -e</tt> or check the exit status of <em>every</em>
- command.
+ Shell scripts (<prgn>sh</prgn> and <prgn>bash</prgn>) other than
+ <file>init.d</file> scripts should almost certainly start
+ with <tt>set -e</tt> so that errors are detected.
+ <file>init.d</file> scripts are something of a special case, due
+ to how frequently they need to call commands that are allowed to
+ fail, and it may instead be easier to check the exit status of
+ commands directly. See <ref id="writing-init"> for more
+ information about writing <file>init.d</file> scripts.
+ </p>
+ <p>
+ Every script should use <tt>set -e</tt> or check the exit status
+ of <em>every</em> command.
</p>
-
<p>
Scripts may assume that <file>/bin/sh</file> implements the
SUSv3 Shell Command Language<footnote>
--
Russ Allbery (r...@debian.org) <http://www.eyrie.org/~eagle/>
--
Raphael Hertzog
> Here is proposed wording, which hopefully reflects the subsequent
> discussion. I'm looking for seconds.
>
Seconded.
Cheers,
--
Raphaël Hertzog
Like what I do? Sponsor me: http://ouaza.com/wp/2010/01/05/5-years-of-freexian/
My Debian goals: http://ouaza.com/wp/2010/01/09/debian-related-goals-for-2010/
Russ Allbery
> On Tue, 2010-06-01 at 10:42:35 -0700, Russ Allbery wrote:
>> Here is proposed wording, which hopefully reflects the subsequent
>> discussion. I'm looking for seconds.
[...]
> Seconded.
Thanks! That's two, so I'm merging for the next release.