Bug#772555: untested hacky attempt at using systemctl preset

[Andreas Henriksson]

Hello Raphael,

On Fri, Mar 04, 2016 at 03:36:54PM +0100, Raphael Hertzog wrote:
> Hello Andreas,
> 
> thanks for working on this patch!
> 
> I want to test your patch in Kali and make it ready for inclusion. But
> there are things that I don't really understand...
> 
> On Wed, 02 Mar 2016, Andreas Henriksson wrote:
> > Please note that perl warns about the smartmatch being experimental.
> > Someone should probably replace that part with a better check....
> 
> 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 = ...

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=).

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.
Now $changed_sth must be set to true so that systemctl daemon-reload
gets called to reload the new status.

> 
> I assume you wanted to compare both data structures and just know whether
> they contain the same thing or not. But the structure is an array of
> hashes. So the smart match compares each hash individually, but the hash
> comparison only ensures that both hashes have the same keys (and does
> not check the values). So basically the smart match will always return
> true.

I'm not sure it's that important to actually detect changes, maybe
we should just hardcode this:

$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...

> 
> But I also don't understand why you compare the result of two consecutive
> get_link_closure() calls. It reads (possibly recursively) the unit file
> given in parameter and builds a structure ouf of it. If the underlying
> unit file has not changed, the two calls should return the same value...
> Even if "systemctl preset" has been called in between, that should not
> have affected files in /lib/systemd/system/
> 
> So I need some explanations on what you tried to do here...

I hope the above explanation helps.

> 
> Still I replaced the smartmatch with a dedicated comparison function
> in case you need it:
> 
> diff --git a/script/deb-systemd-helper b/script/deb-systemd-helper
> index a8aa834..153bed0 100755
> --- a/script/deb-systemd-helper
> +++ b/script/deb-systemd-helper
> @@ -227,6 +227,18 @@ sub is_enabled {
>      return (@missing_links == 0);
>  }
>  
> +sub are_link_closures_different {
> +    my ($list_a, $list_b) = @_;
> +    my $len_a = scalar(@$list_a);
> +    return 1 if $len_a != scalar(@$list_b);
> +    for(my $i = 0; $i < $len_a; $i++) {
> +        my ($hash_a, $hash_b) = ($list_a->[$i], $list_b->[$i]);
> +        return 1 if $hash_a->{'dest'} ne $hash_b->{'dest'};
> +        return 1 if $hash_a->{'src'} ne $hash_b->{'src'};
> +    }
> +    return 0;
> +}
> +
>  sub make_systemd_links {
>      my ($scriptname, $service_path) = @_;
>  
> @@ -251,7 +263,7 @@ sub make_systemd_links {
>          #$changed_sth = ($prev_enabled != $now_enabled);
>          # N.B. Trying a "Smart Matching operator" posing like I actually
>          # know perl or if this even does the right thing.
> -        $changed_sth = !(@prev_links ~~ @links);
> +        $changed_sth = are_link_closures_different(\@prev_links, \@links);
>          # FIXME: We should really update statefile here when we still
>          # know something about the changed links instead of badly
>          # piggy-backing on the fallback which has no idea.
> 
> 
> Cheers,
> -- 
> Raphaël Hertzog ◈ Debian Developer
> 
> Support Debian LTS: http://www.freexian.com/services/debian-lts.html
> Learn to master Debian: http://debian-handbook.info/get/

Regards,
Andreas Henriksson