Bug#772555: untested hacky attempt at using systemctl preset

[Andreas Henriksson]

Hello again,

On Fri, Mar 04, 2016 at 04:30:53PM +0100, Raphael Hertzog wrote:
> Hi,
> 
> On Fri, 04 Mar 2016, Andreas Henriksson wrote:
> > > So I have read about what smartmatch does... but basically I think it
> > > does not do what you wanted to do. So can you tell me what you wanted
> > > to test?
> > 
> > The end result is only used for "changed something" (or not).
> > 
> > See: $changed_sth = ...
> 
> Yes, but it also impacts the "return unless $changed_sth" and thus
> whether we will run the fallback code or not. I think we should always
> run the fallback code...

Yes, that part should also go... Didn't even remember I left that in there.
Leaving it in was probably a mistake to begin with given the comment
just below it (which I wrote later). ;)

> 
> > I left the inital attempt commented out which might help clarify this a
> > bit. See the commented "is_enabled" calls and comparison.
> > I realised that simple comparison is not correct since the unit you're
> > operating on could possibly be unchanged status while something else
> > changed (eg. an underlying unit which got pulled in via Also=).
> 
> Yes, but you need to compare the list of existing symlinks in that case.
> And get_link_closure() doesn't tell you this information.
> 
> > Example: unit foo.service contains Also=bar.service, foo.service is already
> > enabled (because previous versions did not have the Also=bar.service?)
> > while bar.service is not. Running "systemctl preset foo.service" (via
> > updating the foo-package containing foo.service) will lead
> > to bar.service becoming enabled while foo.service status is unchanged.
> 
> So this shows an other actual problem... we should probably not rerun "systemctl
> preset" on package upgrade, do we? Otherwise we will overwrite any change
> made by the local administrator...

I think we have to because when people upgrade their packages which 
might contain service file changes, random package maintainers probably
expect it to just work.... eg. when adding Also=bar.service to their
already existing foo.service.

See also the somewhat related Note in the code and the
"OTOH, was this case even properly handled before?!".
The answer to this is likely 'no'.

> 
> Currently the code creating the symlinks are protected by a check on the
> state file and the call to "systemctl preset" should be similarly
> protected.

The state file only seem to partially protect against doing things
that could potentially have been altered by the local sysadmin.
This link seems to mostly exist to be able to avoid protecting against
rerun deb-systemd-helper.

> 
> > $changed_sth = 1;
> > 
> > This seems to just lead to a "systemctl daemon-reload" which shouldn't
> > be the end of the world if it runs even when not needed.
> > 
> > Making a really convoluted solution just for correctness seems like
> > a bad idea...
> 
> Ack.
> 

Feel free to adjust any of the above mentioned things as you see  fit.
This is getting out of scope for things I can answer. Likely some
init-system-helper maintainer would be better to answer the
"wanted behaviour" type of questions.

Regards,
Andreas Henriksson