[systemd-devel] Crash with extra space after Exec prefix

[Lennart Poettering]

On Wed, 13.05.15 19:11, Martin Pitt (martin.pitt at ubuntu.com) wrote:

> Lennart Poettering [2015-05-13 17:55 +0200]:
> > On Wed, 13.05.15 17:01, Martin Pitt (martin.pitt at ubuntu.com) wrote:
> > > So, obviously we need to fix the crash; but I was wondering what the
> > > desired behaviour should be? In the sense of "be liberal what you
> > > accept" I think the extra space(s) should just be ignored; or should
> > > that count as an error and the unit get rejected?
> > 
> > Neither.
> > 
> > It should be considered an error, logged about, but the line should be
> > ignored and we should continue. This is how we usually do it so far,
> > to ensure unit files stay relatively portable between version, but on
> > the other hand we aren't too liberal with accepting any data.
> 
> You mean ignoring this single line, but still starting the unit (with
> any other Exec*=)? That feels quite odd to me, TBH -- it feels more
> robust if a unit is either completely valid, or completely inert?

Well, for things like PrivateTmp= it's really the behaviour you want:
if we cannot parse it, then we should log about it, but otherwise
ignore it, after all the feature doesn't really have any effect on
execution, it just adds a sandbox. Now, since PrivateTmp= is still
relatively new, it's a good thing if old versions accept newer unit
files where this setting is set or set to an unknown value, without
completely failing, so that people can ship unit files with this,
without completely breaking on old versions.

This is similar for most other options. i.e. If we cannot parse Nice=
or CPUPriority it doesn't really matter either... It's good if it
works, and it should work, but it really shouldn't result in total
failure if we cannot parse it properly.

Now, you could argue that ExecStart= is not of this kind, and that not
having it set properly should cause failure. But that's the case for
ExecStart= only, for thing things like ExecStartPre= and friends it
might already be quite different, they tend to be much less essential.

Also, and most importantly, note that after parsing the unit file,
systemd will actually do a final verification check at the time it
marks the unit as "fully loaded". Then, it will verify that an
ExecStart= has actually been set, and if it is missing it will
indicate this in the LoadState field, for clients to query.

Or in other words: yes, some bits are essential to be set, but those
should be checked for after parsing is complete, not already while
parsing.

Lennart

-- 
Lennart Poettering, Red Hat