From db20c5cec8adf865dd47672bc091092b8cea5e0e Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

Date: Thu, 10 Mar 2022 15:47:12 +0100

Subject: [PATCH] shared/install: return failure when enablement fails, but

 process as much as possible

So far we'd issue a warning (before this series, just in the logs on the server

side, and before this commit, on stderr on the caller's side), but return

success. It seems that successfull return was introduced by mistake in

aa0f357fd833feecbea6c3e9be80b643e433bced (my fault :( ), which was supposed to

be a refactoring without a functional change. I think it's better to fail,

because if enablement fails, the user will most likely want to diagnose the

issue.

Note that we still do partial enablement, as far as that is possible. So if

e.g. we have [Install] Alias=foo.service foobar, we'll create the symlink

'foo.service', but not 'foobar', since that's not a valid unit name. We'll

print info about the action taken, and about 'foobar' being invalid, and return

failure.

(cherry picked from commit 0d11db59825a9deee0b56fdede0602ef1c37c5c5)

Related: #2082131

---

 src/shared/install.c          | 10 +++++----

 test/test-systemctl-enable.sh | 39 ++++++++++++++++++-----------------

 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/src/shared/install.c b/src/shared/install.c

index 6da9ba6b0c..a541d32fb7 100644

--- a/src/shared/install.c

+++ b/src/shared/install.c

@@ -1802,20 +1802,22 @@ static int install_info_symlink_alias(

                 q = install_name_printf(scope, info, *s, info->root, &dst);

                 if (q < 0) {

                         unit_file_changes_add(changes, n_changes, q, *s, NULL);

-                        return q;

+                        r = r < 0 ? r : q;

+                        continue;

                 }

                 q = unit_file_verify_alias(info, dst, &dst_updated, changes, n_changes);

-                if (q < 0)

+                if (q < 0) {

+                        r = r < 0 ? r : q;

                         continue;

+                }

                 alias_path = path_make_absolute(dst_updated ?: dst, config_path);

                 if (!alias_path)

                         return -ENOMEM;

                 q = create_symlink(lp, info->path, alias_path, force, changes, n_changes);

-                if (r == 0)

-                        r = q;

+                r = r < 0 ? r : q;

         }

         return r;

diff --git a/test/test-systemctl-enable.sh b/test/test-systemctl-enable.sh

index 8ac1342b91..32bc6e5ef7 100644

--- a/test/test-systemctl-enable.sh

+++ b/test/test-systemctl-enable.sh

@@ -56,19 +56,27 @@ test ! -e "$root/etc/systemd/system/default.target.wants/test1.service"

 test ! -e "$root/etc/systemd/system/special.target.requires/test1.service"

 : -------aliases----------------------------------------------

-"$systemctl" --root="$root" enable test1

-test -h "$root/etc/systemd/system/default.target.wants/test1.service"

-test -h "$root/etc/systemd/system/special.target.requires/test1.service"

-

 cat >>"$root/etc/systemd/system/test1.service" <

 Alias=test1-goodalias.service

 Alias=test1@badalias.service

 Alias=test1-badalias.target

 Alias=test1-badalias.socket

+# we have a series of good, bad, and then good again

+Alias=test1-goodalias2.service

 EOF

+"$systemctl" --root="$root" enable test1 && { echo "Expected failure" >&2; exit 1; }

+test -h "$root/etc/systemd/system/default.target.wants/test1.service"

+test -h "$root/etc/systemd/system/special.target.requires/test1.service"

+test ! -e "$root/etc/systemd/system/test1-goodalias.service"

+test -h "$root/etc/systemd/system/test1-goodalias.service"

+test ! -e "$root/etc/systemd/system/test1@badalias.service"

+test ! -e "$root/etc/systemd/system/test1-badalias.target"

+test ! -e "$root/etc/systemd/system/test1-badalias.socket"

+test -h "$root/etc/systemd/system/test1-goodalias2.service"

+

 : -------aliases in reeanble----------------------------------

-"$systemctl" --root="$root" reenable test1

+"$systemctl" --root="$root" reenable test1 && { echo "Expected failure" >&2; exit 1; }

 test -h "$root/etc/systemd/system/default.target.wants/test1.service"

 test ! -e "$root/etc/systemd/system/test1-goodalias.service"

 test -h "$root/etc/systemd/system/test1-goodalias.service"

@@ -328,7 +336,7 @@ Alias=link4alias.service

 Alias=link4alias2.service

 EOF

-"$systemctl" --root="$root" enable 'link4.service'

+"$systemctl" --root="$root" enable 'link4.service' && { echo "Expected failure" >&2; exit 1; }

 test ! -h "$root/etc/systemd/system/link4.service"  # this is our file

 test ! -h "$root/etc/systemd/system/link4@.service"

 test ! -h "$root/etc/systemd/system/link4@inst.service"

@@ -343,18 +351,20 @@ test ! -h "$root/etc/systemd/system/link4alias.service"

 test ! -h "$root/etc/systemd/system/link4alias2.service"

 : -------systemctl enable on path to unit file----------------

+cat >"$root/etc/systemd/system/link4.service" <

+[Install]

+Alias=link4alias.service

+Alias=link4alias2.service

+EOF

+

 # Apparently this works. I'm not sure what to think.

 "$systemctl" --root="$root" enable '/etc/systemd/system/link4.service'

 test ! -h "$root/etc/systemd/system/link4.service"  # this is our file

-test ! -h "$root/etc/systemd/system/link4@.service"

-test ! -h "$root/etc/systemd/system/link4@inst.service"

 islink "$root/etc/systemd/system/link4alias.service" "/etc/systemd/system/link4.service"

 islink "$root/etc/systemd/system/link4alias2.service" "/etc/systemd/system/link4.service"

 "$systemctl" --root="$root" disable '/etc/systemd/system/link4.service'

 test ! -h "$root/etc/systemd/system/link4.service"

-test ! -h "$root/etc/systemd/system/link4@.service"

-test ! -h "$root/etc/systemd/system/link4@inst.service"

 test ! -h "$root/etc/systemd/system/link4alias.service"

 test ! -h "$root/etc/systemd/system/link4alias2.service"

@@ -364,25 +374,18 @@ cat >"$root/etc/systemd/system/link5.service" <

 [Install]

 # FIXME: self-alias should be ignored

 # Alias=link5.service

-Alias=link5@.service

-Alias=link5@inst.service

 Alias=link5alias.service

 Alias=link5alias2.service

 EOF

 "$systemctl" --root="$root" enable 'link5.service'

 test ! -h "$root/etc/systemd/system/link5.service"  # this is our file

-test ! -h "$root/etc/systemd/system/link5@.service"

-test ! -h "$root/etc/systemd/system/link5@inst.service"

 # FIXME/CLARIFYME: will systemd think that link5alias2, link5alias, link5 are all aliases?

 # https://github.com/systemd/systemd/issues/661#issuecomment-1057931149

 islink "$root/etc/systemd/system/link5alias.service" "/etc/systemd/system/link5.service"

 islink "$root/etc/systemd/system/link5alias2.service" "/etc/systemd/system/link5.service"

 "$systemctl" --root="$root" disable 'link5.service'

-test ! -h "$root/etc/systemd/system/link5.service"

-test ! -h "$root/etc/systemd/system/link5@.service"

-test ! -h "$root/etc/systemd/system/link5@inst.service"

 test ! -h "$root/etc/systemd/system/link5alias.service"

 test ! -h "$root/etc/systemd/system/link5alias2.service"

@@ -528,8 +531,6 @@ check_alias % '%' && { echo "Expected failure because % is not legal in unit nam

 check_alias z 'z' && { echo "Expected failure because %z is not known" >&2; exit 1; }

-# FIXME: if there's an invalid Alias=, we shouldn't preach about empty [Install]

-

 # TODO: repeat the tests above for presets

 : -------SYSTEMD_OS_RELEASE relative to root------------------