Bug#768456: Second version of the patch

[Martin Pitt]

Hey Didier,

thanks for fixing this! The general idea looks good to me, I just have
some nitpicks which the Debian maintainers may or may not want to see
changed.

Didier Roche [2014-11-07 16:36 +0100]:
> +# If the job is disabled and is not currently running, the job is not started or restarted.
> +# However, if the job is disabled but has been forced into the running state, we *do* stop
> +# and restart it since this is expected behaviour for the admin who forced the start.
> +if (grep("/^$action$/", ["start", "restart"])) {

I know, regexps are the bread and butter of perl, but wouldn't

  if ($action eq 'start' || $action eq 'restart') {

be a tad easier/robust?

> +        if (($unit_enabled != 0) && ("$action" eq "start")) {
> +            print STDERR "$unit is disabled, don't start it.\n";

"don't start it" sounds like a request to the caller; could this say
"not starting it"?

> +        } elsif (($unit_enabled != 0) && ($unit_active != 0) && ("$action" eq "restart")) {
> +            print STDERR "$unit is disabled and not running, don't start it.\n";

Dito.

> +        else {
> +            system('/bin/systemctl', "$action", "$unit");

This should use exec like below IMHO, so that the exit status of
deb-systemd-invoke is the exit status of systemctl.

> +        }
> +    }
> +} else {
> +    exec '/bin/systemctl', @ARGV;

Merci !

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)