core: introduce a new load state "bad-setting"

Since bb28e68477a3a39796e4999a6cbc6ac6345a9159 parsing failures of
certain unit file settings will result in load failures of units. This
introduces a new load state "bad-setting" that is entered in precisely
this case.

With this addition error messages on bad settings should be a lot more
explicit, as we don't have to show some generic "errno" error in that
case, but can explicitly say that a bad setting is at fault.

Internally this unit load state is entered as soon as any configuration
loader call returns ENOEXEC. Hence: config parser calls should return
ENOEXEC now for such essential unit file settings. Turns out, they
generally already do.

Fixes: #9107

9 files changed, 38 insertions, 22 deletions

diff --git a/man/systemctl.xml b/man/systemctl.xml
index 1237962b3c..60441ff7ab 100644
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -702,13 +702,14 @@ To show all installed unit files use 'systemctl list-unit-files'.
             were masked, not found, or otherwise failed.</para>
 
             <para>The LOAD column shows the load state, one of <constant>loaded</constant>,
-            <constant>not-found</constant>, <constant>error</constant>, <constant>masked</constant>. The ACTIVE columns
-            shows the general unit state, one of <constant>active</constant>, <constant>reloading</constant>,
-            <constant>inactive</constant>, <constant>failed</constant>, <constant>activating</constant>,
-            <constant>deactivating</constant>. The SUB column shows the unit-type-specific detailed state of the unit,
-            possible values vary by unit type. The list of possible LOAD, ACTIVE, and SUB states is not constant and
-            new systemd releases may both add and remove values. <programlisting>systemctl --state=help</programlisting> command maybe be
-            used to display the current set of possible values.</para>
+            <constant>not-found</constant>, <constant>bad-setting</constant>, <constant>error</constant>,
+            <constant>masked</constant>. The ACTIVE columns shows the general unit state, one of
+            <constant>active</constant>, <constant>reloading</constant>, <constant>inactive</constant>,
+            <constant>failed</constant>, <constant>activating</constant>, <constant>deactivating</constant>. The SUB
+            column shows the unit-type-specific detailed state of the unit, possible values vary by unit type. The list
+            of possible LOAD, ACTIVE, and SUB states is not constant and new systemd releases may both add and remove
+            values. <programlisting>systemctl --state=help</programlisting> command maybe be used to display the
+            current set of possible values.</para>
 
             <para>This is the default command.</para>
           </listitem>
@@ -970,10 +971,12 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err
 
             <para>The "Loaded:" line in the output will show <literal>loaded</literal> if the unit has been loaded into
             memory. Other possible values for "Loaded:" include: <literal>error</literal> if there was a problem
-            loading it, <literal>not-found</literal>, and <literal>masked</literal>. Along with showing the path to
-            the unit file, this line will also show the enablement state.  Enabled commands start at boot.  See the
-            full table of possible enablement states — including the definition of <literal>masked</literal> — in the
-            documentation for the <command>is-enabled</command> command.
+            loading it, <literal>not-found</literal> if not unit file was found for this unit,
+            <literal>bad-setting</literal> if an essential unit file setting could not be parsed and
+            <literal>masked</literal> if the unit file has been masked. Along with showing the path to the unit file,
+            this line will also show the enablement state.  Enabled commands start at boot.  See the full table of
+            possible enablement states — including the definition of <literal>masked</literal> — in the documentation
+            for the <command>is-enabled</command> command.
             </para>
 
             <para>The "Active:" line shows active state.  The value is usually <literal>active</literal> or
diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c
index 7361acb297..fd42e0a241 100644
--- a/src/basic/unit-def.c
+++ b/src/basic/unit-def.c
@@ -93,6 +93,7 @@ static const char* const unit_load_state_table[_UNIT_LOAD_STATE_MAX] = {
         [UNIT_STUB] = "stub",
         [UNIT_LOADED] = "loaded",
         [UNIT_NOT_FOUND] = "not-found",
+        [UNIT_BAD_SETTING] = "bad-setting",
         [UNIT_ERROR] = "error",
         [UNIT_MERGED] = "merged",
         [UNIT_MASKED] = "masked"
diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h
index 503867668b..3f13a1c8f3 100644
--- a/src/basic/unit-def.h
+++ b/src/basic/unit-def.h
@@ -30,8 +30,9 @@ typedef enum UnitType {
 typedef enum UnitLoadState {
         UNIT_STUB = 0,
         UNIT_LOADED,
-        UNIT_NOT_FOUND,
-        UNIT_ERROR,
+        UNIT_NOT_FOUND,    /* error condition #1: unit file not found */
+        UNIT_BAD_SETTING,  /* error condition #2: we couldn't parse some essential unit file setting */
+        UNIT_ERROR,        /* error condition #3: other "system" error, catchall for the rest */
         UNIT_MERGED,
         UNIT_MASKED,
         _UNIT_LOAD_STATE_MAX,
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index a728dbe670..d0910e9dc3 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -1242,7 +1242,7 @@ int bus_unit_queue_job(
         }
 
         if (type == JOB_STOP &&
-            (IN_SET(u->load_state, UNIT_NOT_FOUND, UNIT_ERROR)) &&
+            IN_SET(u->load_state, UNIT_NOT_FOUND, UNIT_ERROR, UNIT_BAD_SETTING) &&
             unit_active_state(u) == UNIT_INACTIVE)
                 return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s not loaded.", u->id);
 
@@ -1727,6 +1727,9 @@ int bus_unit_validate_load_state(Unit *u, sd_bus_error *error) {
         case UNIT_NOT_FOUND:
                 return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s not found.", u->id);
 
+        case UNIT_BAD_SETTING:
+                return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, "Unit %s has a bad unit file setting.", u->id);
+
         case UNIT_ERROR: /* Only show .load_error in UNIT_ERROR state */
                 return sd_bus_error_set_errnof(error, u->load_error, "Unit %s failed to loaded properly: %m.", u->id);
 
diff --git a/src/core/transaction.c b/src/core/transaction.c
index 516e5e9a84..3dfc99819b 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -906,7 +906,9 @@ int transaction_add_job_and_dependencies(
         /*           by ? by->unit->id : "NA", */
         /*           by ? job_type_to_string(by->type) : "NA"); */
 
-        if (!IN_SET(unit->load_state, UNIT_LOADED, UNIT_ERROR, UNIT_NOT_FOUND, UNIT_MASKED))
+        /* Safety check that the unit is a valid state, i.e. not in UNIT_STUB or UNIT_MERGED which should only be set
+         * temporarily. */
+        if (!IN_SET(unit->load_state, UNIT_LOADED, UNIT_ERROR, UNIT_NOT_FOUND, UNIT_BAD_SETTING, UNIT_MASKED))
                 return sd_bus_error_setf(e, BUS_ERROR_LOAD_FAILED, "Unit %s is not loaded properly.", unit->id);
 
         if (type != JOB_STOP) {
diff --git a/src/core/unit.c b/src/core/unit.c
index a82e5fd9eb..d21c09cf15 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1528,14 +1528,18 @@ int unit_load(Unit *u) {
         return 0;
 
 fail:
-        u->load_state = u->load_state == UNIT_STUB ? UNIT_NOT_FOUND : UNIT_ERROR;
+        /* We convert ENOEXEC errors to the UNIT_BAD_SETTING load state here. Configuration parsing code should hence
+         * return ENOEXEC to ensure units are placed in this state after loading */
+
+        u->load_state = u->load_state == UNIT_STUB ? UNIT_NOT_FOUND :
+                                     r == -ENOEXEC ? UNIT_BAD_SETTING :
+                                                     UNIT_ERROR;
         u->load_error = r;
+
         unit_add_to_dbus_queue(u);
         unit_add_to_gc_queue(u);
 
-        log_unit_debug_errno(u, r, "Failed to load configuration: %m");
-
-        return r;
+        return log_unit_debug_errno(u, r, "Failed to load configuration: %m");
 }
 
 static bool unit_condition_test_list(Unit *u, Condition *first, const char *(*to_string)(ConditionType t)) {
diff --git a/src/libsystemd/sd-bus/bus-common-errors.c b/src/libsystemd/sd-bus/bus-common-errors.c
index 9d9765e568..b815f2f2eb 100644
--- a/src/libsystemd/sd-bus/bus-common-errors.c
+++ b/src/libsystemd/sd-bus/bus-common-errors.c
@@ -18,6 +18,7 @@ BUS_ERROR_MAP_ELF_REGISTER const sd_bus_error_map bus_common_errors[] = {
         SD_BUS_ERROR_MAP(BUS_ERROR_NO_UNIT_FOR_INVOCATION_ID,    ENOENT),
         SD_BUS_ERROR_MAP(BUS_ERROR_UNIT_EXISTS,                  EEXIST),
         SD_BUS_ERROR_MAP(BUS_ERROR_LOAD_FAILED,                  EIO),
+        SD_BUS_ERROR_MAP(BUS_ERROR_BAD_UNIT_SETTING,             ENOEXEC),
         SD_BUS_ERROR_MAP(BUS_ERROR_JOB_FAILED,                   EREMOTEIO),
         SD_BUS_ERROR_MAP(BUS_ERROR_NO_SUCH_JOB,                  ENOENT),
         SD_BUS_ERROR_MAP(BUS_ERROR_NOT_SUBSCRIBED,               EINVAL),
diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h
index c67015c2c9..e34aedc528 100644
--- a/src/libsystemd/sd-bus/bus-common-errors.h
+++ b/src/libsystemd/sd-bus/bus-common-errors.h
@@ -14,6 +14,7 @@
 #define BUS_ERROR_NO_UNIT_FOR_INVOCATION_ID "org.freedesktop.systemd1.NoUnitForInvocationID"
 #define BUS_ERROR_UNIT_EXISTS "org.freedesktop.systemd1.UnitExists"
 #define BUS_ERROR_LOAD_FAILED "org.freedesktop.systemd1.LoadFailed"
+#define BUS_ERROR_BAD_UNIT_SETTING "org.freedesktop.systemd1.BadUnitSetting"
 #define BUS_ERROR_JOB_FAILED "org.freedesktop.systemd1.JobFailed"
 #define BUS_ERROR_NO_SUCH_JOB "org.freedesktop.systemd1.NoSuchJob"
 #define BUS_ERROR_NOT_SUBSCRIBED "org.freedesktop.systemd1.NotSubscribed"
diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index 49b00a77ea..d5ed5c2e2a 100644
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -416,7 +416,7 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
 
                 if (!arg_no_legend &&
                     (streq(u->active_state, "failed") ||
-                     STR_IN_SET(u->load_state, "error", "not-found", "masked")))
+                     STR_IN_SET(u->load_state, "error", "not-found", "bad-setting", "masked")))
                         circle_len = 2;
         }
 
@@ -493,7 +493,7 @@ static int output_units_list(const UnitInfo *unit_infos, unsigned c) {
                         underline = true;
                 }
 
-                if (STR_IN_SET(u->load_state, "error", "not-found", "masked") && !arg_plain) {
+                if (STR_IN_SET(u->load_state, "error", "not-found", "bad-setting", "masked") && !arg_plain) {
                         on_circle = ansi_highlight_yellow();
                         off_circle = ansi_normal();
                         circle = true;
@@ -3978,7 +3978,7 @@ static void print_status_info(
         if (i->following)
                 printf("   Follow: unit currently follows state of %s\n", i->following);
 
-        if (streq_ptr(i->load_state, "error")) {
+        if (STRPTR_IN_SET(i->load_state, "error", "not-found", "bad-setting")) {
                 on = ansi_highlight_red();
                 off = ansi_normal();
         } else