shared/install,systemctl,core: report offending file on installation error

Fixes #2191:

$ systemctl --root=/ enable sddm
Created symlink /etc/systemd/system/display-manager.service, pointing to /usr/lib/systemd/system/sddm.service.
$ sudo build/systemctl --root=/ enable gdm
Failed to enable unit, file /etc/systemd/system/display-manager.service already exists and is a symlink to /usr/lib/systemd/system/sddm.service.
$ sudo build/systemctl --root= enable sddm
$ sudo build/systemctl --root= enable gdm
Failed to enable unit: File /etc/systemd/system/display-manager.service already exists and is a symlink to /usr/lib/systemd/system/sddm.service.

(I tried a few different approaches to pass the error information back to the
caller. Adding a new parameter to hold the error results in a gigantic patch
and a lot of hassle to pass the args arounds. Adding this information to the
changes array is straightforward and can be more easily extended in the
future.)

In case local installation is performed, the full set of errors can be reported
and we do that. When running over dbus, only the first error is reported.

1 files changed, 74 insertions, 45 deletions

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index ddfde23028..d48b0ca69d 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1585,15 +1585,19 @@ static int reply_unit_file_changes_and_free(
         if (r < 0)
                 goto fail;
 
-        for (i = 0; i < n_changes; i++) {
-                r = sd_bus_message_append(
-                                reply, "(sss)",
-                                unit_file_change_type_to_string(changes[i].type),
-                                changes[i].path,
-                                changes[i].source);
-                if (r < 0)
-                        goto fail;
-        }
+        for (i = 0; i < n_changes; i++)
+                if (changes[i].type >= 0) {
+                        const char *change = unit_file_change_type_to_string(changes[i].type);
+                        assert(change != NULL);
+
+                        r = sd_bus_message_append(
+                                        reply, "(sss)",
+                                        change,
+                                        changes[i].path,
+                                        changes[i].source);
+                        if (r < 0)
+                                goto fail;
+                }
 
         r = sd_bus_message_close_container(reply);
         if (r < 0)
@@ -1607,17 +1611,56 @@ fail:
         return r;
 }
 
-static int install_error(sd_bus_error *error, int c) {
+/* Create an error reply, using the error information from changes[]
+ * if possible, and fall back to generating an error from error code c.
+ * The error message only describes the first error.
+ *
+ * Coordinate with unit_file_dump_changes() in install.c.
+ */
+static int install_error(
+                sd_bus_error *error,
+                int c,
+                UnitFileChange *changes,
+                unsigned n_changes) {
+        int r;
+        unsigned i;
         assert(c < 0);
 
-        if (c == -ERFKILL)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED, "Unit file is masked.");
-        if (c == -EADDRNOTAVAIL)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED, "Unit file is transient or generated.");
-        if (c == -ELOOP)
-                return sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED, "Refusing to operate on linked unit file.");
+        for (i = 0; i < n_changes; i++)
+                switch(changes[i].type) {
+                case 0 ... INT_MAX:
+                        continue;
+                case -EEXIST:
+                        if (changes[i].source)
+                                r = sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS,
+                                                      "File %s already exists and is a symlink to %s.",
+                                                      changes[i].path, changes[i].source);
+                        else
+                                r = sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS,
+                                                      "File %s already exists.",
+                                                      changes[i].path);
+                        goto found;
+                case -ERFKILL:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED,
+                                              "Unit file %s is masked.", changes[i].path);
+                        goto found;
+                case -EADDRNOTAVAIL:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED,
+                                              "Unit %s is transient or generated.", changes[i].path);
+                        goto found;
+                case -ELOOP:
+                        r = sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED,
+                                              "Refusing to operate on linked unit file %s", changes[i].path);
+                        goto found;
+                default:
+                        r = sd_bus_error_set_errnof(error, changes[i].type, "File %s: %m", changes[i].path);
+                        goto found;
+                }
 
-        return c;
+        r = c;
+ found:
+        unit_file_changes_free(changes, n_changes);
+        return r;
 }
 
 static int method_enable_unit_files_generic(
@@ -1651,10 +1694,8 @@ static int method_enable_unit_files_generic(
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = call(m->unit_file_scope, runtime, NULL, l, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, carries_install_info ? r : -1, changes, n_changes);
 }
@@ -1719,10 +1760,8 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_preset(m->unit_file_scope, runtime, NULL, l, mm, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, r, changes, n_changes);
 }
@@ -1757,10 +1796,8 @@ static int method_disable_unit_files_generic(
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = call(m->unit_file_scope, runtime, NULL, l, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1794,10 +1831,8 @@ static int method_revert_unit_files(sd_bus_message *message, void *userdata, sd_
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_revert(m->unit_file_scope, NULL, l, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1827,10 +1862,8 @@ static int method_set_default_target(sd_bus_message *message, void *userdata, sd
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_set_default(m->unit_file_scope, NULL, name, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1869,10 +1902,8 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata,
                 return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
 
         r = unit_file_preset_all(m->unit_file_scope, runtime, NULL, mm, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }
@@ -1908,10 +1939,8 @@ static int method_add_dependency_unit_files(sd_bus_message *message, void *userd
                 return -EINVAL;
 
         r = unit_file_add_dependency(m->unit_file_scope, runtime, NULL, l, target, dep, force, &changes, &n_changes);
-        if (r < 0) {
-                unit_file_changes_free(changes, n_changes);
-                return install_error(error, r);
-        }
+        if (r < 0)
+                return install_error(error, r, changes, n_changes);
 
         return reply_unit_file_changes_and_free(m, message, -1, changes, n_changes);
 }