Bug#892357: deb-systemd-invoke: if one unit is forbidden by policy-rc.d, all requested units are skipped

[Colin Watson]

Package: init-system-helpers
Version: 1.51
Severity: normal

dh_systemd_start generates a deb-systemd-invoke command listing all the
binary package's units.  For snapd in Ubuntu, for example, this looks
like this:

  deb-systemd-invoke start snapd.autoimport.service snapd.core-fixup.service snapd.refresh.service snapd.refresh.timer snapd.service snapd.snap-repair.service snapd.snap-repair.timer snapd.socket snapd.system-shutdown.service >/dev/null || true

(I'm sure there are other similar examples of a package shipping many
services; this is just the one I have to hand right now.)

Now, in some situations one might well want to disable a subset of those
units using the policy-rc.d interface.  For example, in a buildd-type
situation, it makes sense to disable snapd.refresh.timer.  Unfortunately
this can't be done using policy-rc.d because deb-systemd-invoke handles
policy-rc.d like this:

  if (-x $policyhelper) {
      for my $unit (@units) {
          system(qq|$policyhelper $unit "$action"|);
  
          # 0 or 104 means run
          # 101 means do not run
          my $exitcode = ($? >> 8);
          if ($exitcode == 101) {
              print STDERR "$policyhelper returned 101, not running '" . join(' ', @ARGV) . "'\n";
              exit 0;
          } elsif ($exitcode != 104 && $exitcode != 0) {
              print STDERR "deb-systemd-invoke only supports $policyhelper return codes 0, 101, and 104!\n";
              print STDERR "Got return code $exitcode, ignoring.\n";
          }
      }
  }

Thus, if policy-rc.d returns 101 for any single unit, they all fail to
start (and so it's necessary to resort to alternative hacks like
symlinking /etc/systemd/system/snapd.refresh.timer to /dev/null).  This
seems at best somewhat counterintuitive.

Would it be possible instead for deb-systemd-invoke to handle this in a
finer-grained way?  I'm thinking of something like the following,
although it's untested beyond a syntax check:

diff --git a/script/deb-systemd-invoke b/script/deb-systemd-invoke
index 71b1c33..ba76cba 100755
--- a/script/deb-systemd-invoke
+++ b/script/deb-systemd-invoke
@@ -61,6 +61,7 @@ my $policyhelper = '/usr/sbin/policy-rc.d';
 my @units = @ARGV;
 my $action = shift @units;
 if (-x $policyhelper) {
+    my @allowed_units;
     for my $unit (@units) {
         system(qq|$policyhelper $unit "$action"|);
 
@@ -68,13 +69,16 @@ if (-x $policyhelper) {
         # 101 means do not run
         my $exitcode = ($? >> 8);
         if ($exitcode == 101) {
-            print STDERR "$policyhelper returned 101, not running '" . join(' ', @ARGV) . "'\n";
-            exit 0;
+            print STDERR "$policyhelper returned 101, not running '$unit'\n";
         } elsif ($exitcode != 104 && $exitcode != 0) {
             print STDERR "deb-systemd-invoke only supports $policyhelper return codes 0, 101, and 104!\n";
             print STDERR "Got return code $exitcode, ignoring.\n";
+            push @allowed_units, $unit;
+        } else {
+            push @allowed_units, $unit;
         }
     }
+    @units = @allowed_units;
 }
 
 # If the job is disabled and is not currently running, the job is not started or restarted.
@@ -107,5 +111,5 @@ if ($action eq "start" || $action eq "restart") {
     }
     exit(0);
 } else {
-    exec '/bin/systemctl', @ARGV;
+    exec '/bin/systemctl', @units if @units;
 }

Thanks,

-- 
Colin Watson                                       [cjwatson at debian.org]