Merge pull request #13754 from claudiozz/master

Allow restart for oneshot units

man/systemctl.xml

@@ -155,14 +155,21 @@ Sun 2017-02-26 20:57:49 EST  2h 3min left  Sun 2017-02-26 11:56:36 EST  6h ago
           <term><command>start <replaceable>PATTERN</replaceable>…</command></term>
 
           <listitem>
-            <para>Start (activate) one or more units specified on the
+            <para>Start (activate) one or more units specified on the command line.</para>
-            command line.</para>
 
-            <para>Note that glob patterns operate on the set of primary names of units currently in memory. Units which
+            <para>Note that unit glob patterns expand to names of units currently in memory. Units which are
-            are not active and are not in a failed state usually are not in memory, and will not be matched by any
+            not active and are not in a failed state usually are not in memory, and will not be matched by
-            pattern. In addition, in case of instantiated units, systemd is often unaware of the instance name until
+            any pattern. In addition, in case of instantiated units, systemd is often unaware of the instance
-            the instance has been started. Therefore, using glob patterns with <command>start</command> has limited
+            name until the instance has been started. Therefore, using glob patterns with
-            usefulness. Also, secondary alias names of units are not considered.</para>
+            <command>start</command> has limited usefulness. Also, secondary alias names of units are not
+            considered.</para>
+
+            <para>Option <option>--all</option> may be used to also operate on inactive units which are
+            referenced by other loaded units. Note that this is not the same as operating on "all" possible
+            units, because as the previous paragraph describes, such a list is ill-defined. Nevertheless,
+            <command>systemctl start --all <replaceable>GLOB</replaceable></command> may be useful if all the
+            units that should match the pattern are pulled in by some target which is known to be loaded.
+            </para>
           </listitem>
         </varlistentry>
         <varlistentry>

man/systemd.service.xml

@@ -1328,9 +1328,10 @@ WantedBy=multi-user.target</programlisting>
 
       <para><varname>Type=</varname><option>oneshot</option> are the
       only service units that may have more than one
-      <varname>ExecStart=</varname> specified. They will be executed
+      <varname>ExecStart=</varname> specified. For units with multiple
-      in order until either they are all successful or one of them
+      commands (<varname noindex="true">Type=oneshot</varname>), all commands will be run again.</para>
-      fails.</para>
+      <para> For <varname noindex="true">Type=oneshot</varname>, <varname>Restart=</varname><option>always</option>
+      and <varname>Restart=</varname><option>on-success</option> are <emphasis>not</emphasis> allowed.</para>
     </example>
 
     <example>

src/core/service.c

@@ -575,8 +575,9 @@ static int service_verify(Service *s) {
                 return -ENOEXEC;
         }
 
-        if (s->type == SERVICE_ONESHOT && s->restart != SERVICE_RESTART_NO) {
+        if (s->type == SERVICE_ONESHOT
-                log_unit_error(UNIT(s), "Service has Restart= setting other than no, which isn't allowed for Type=oneshot services. Refusing.");
+            && !IN_SET(s->restart, SERVICE_RESTART_NO, SERVICE_RESTART_ON_FAILURE, SERVICE_RESTART_ON_ABNORMAL, SERVICE_RESTART_ON_WATCHDOG, SERVICE_RESTART_ON_ABORT)) {
+                log_unit_error(UNIT(s), "Service has Restart= set to either always or on-success, which isn't allowed for Type=oneshot services. Refusing.");
                 return -ENOEXEC;
         }
 

src/systemctl/systemctl.c

@@ -749,7 +749,7 @@ static int get_unit_list_recursive(
         return c;
 }
 
-static int expand_names(sd_bus *bus, char **names, const char* suffix, char ***ret) {
+static int expand_names(sd_bus *bus, char **names, const char* suffix, char ***ret, bool *ret_expanded) {
         _cleanup_strv_free_ char **mangled = NULL, **globs = NULL;
         char **name;
         int r, i;
@@ -778,7 +778,8 @@ static int expand_names(sd_bus *bus, char **names, const char* suffix, char ***r
 
         /* Query the manager only if any of the names are a glob, since
          * this is fairly expensive */
-        if (!strv_isempty(globs)) {
+        bool expanded = !strv_isempty(globs);
+        if (expanded) {
                 _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
                 _cleanup_free_ UnitInfo *unit_infos = NULL;
                 size_t allocated, n;
@@ -802,6 +803,9 @@ static int expand_names(sd_bus *bus, char **names, const char* suffix, char ***r
                 }
         }
 
+        if (ret_expanded)
+                *ret_expanded = expanded;
+
         *ret = TAKE_PTR(mangled);
         return 0;
 }
@@ -1033,7 +1037,7 @@ static int list_sockets(int argc, char *argv[], void *userdata) {
 
         (void) pager_open(arg_pager_flags);
 
-        r = expand_names(bus, strv_skip(argv, 1), ".socket", &sockets_with_suffix);
+        r = expand_names(bus, strv_skip(argv, 1), ".socket", &sockets_with_suffix, NULL);
         if (r < 0)
                 return r;
 
@@ -1345,7 +1349,7 @@ static int list_timers(int argc, char *argv[], void *userdata) {
 
         (void) pager_open(arg_pager_flags);
 
-        r = expand_names(bus, strv_skip(argv, 1), ".timer", &timers_with_suffix);
+        r = expand_names(bus, strv_skip(argv, 1), ".timer", &timers_with_suffix, NULL);
         if (r < 0)
                 return r;
 
@@ -3118,9 +3122,20 @@ static int start_unit(int argc, char *argv[], void *userdata) {
                 if (!names)
                         return log_oom();
         } else {
-                r = expand_names(bus, strv_skip(argv, 1), suffix, &names);
+                bool expanded;
+
+                r = expand_names(bus, strv_skip(argv, 1), suffix, &names, &expanded);
                 if (r < 0)
                         return log_error_errno(r, "Failed to expand names: %m");
+
+                if (!arg_all && expanded && streq(job_type, "start") && !arg_quiet) {
+                        log_warning("Warning: %ssystemctl start called with a glob pattern.%s",
+                                    ansi_highlight_red(),
+                                    ansi_normal());
+                        log_notice("Hint: unit globs expand to loaded units, so start will usually have no effect.\n"
+                                   "      Passing --all will also load units which are pulled in by other units.\n"
+                                   "      See systemctl(1) for more details.");
+                }
         }
 
         if (!arg_no_block) {
@@ -3728,7 +3743,7 @@ static int check_unit_generic(int code, const UnitActiveState good_states[], int
         if (r < 0)
                 return r;
 
-        r = expand_names(bus, args, NULL, &names);
+        r = expand_names(bus, args, NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -3787,7 +3802,7 @@ static int kill_unit(int argc, char *argv[], void *userdata) {
         if (streq(arg_job_mode, "fail"))
                 kill_who = strjoina(arg_kill_who, "-fail");
 
-        r = expand_names(bus, strv_skip(argv, 1), NULL, &names);
+        r = expand_names(bus, strv_skip(argv, 1), NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -3832,7 +3847,7 @@ static int clean_unit(int argc, char *argv[], void *userdata) {
                         return log_oom();
         }
 
-        r = expand_names(bus, strv_skip(argv, 1), NULL, &names);
+        r = expand_names(bus, strv_skip(argv, 1), NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -5860,7 +5875,7 @@ static int show(int argc, char *argv[], void *userdata) {
                 if (!strv_isempty(patterns)) {
                         _cleanup_strv_free_ char **names = NULL;
 
-                        r = expand_names(bus, patterns, NULL, &names);
+                        r = expand_names(bus, patterns, NULL, &names, NULL);
                         if (r < 0)
                                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -5910,7 +5925,7 @@ static int cat(int argc, char *argv[], void *userdata) {
         if (r < 0)
                 return r;
 
-        r = expand_names(bus, strv_skip(argv, 1), NULL, &names);
+        r = expand_names(bus, strv_skip(argv, 1), NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -6141,7 +6156,7 @@ static int reset_failed(int argc, char *argv[], void *userdata) {
 
         polkit_agent_open_maybe();
 
-        r = expand_names(bus, strv_skip(argv, 1), NULL, &names);
+        r = expand_names(bus, strv_skip(argv, 1), NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 
@@ -7575,7 +7590,7 @@ static int edit(int argc, char *argv[], void *userdata) {
         if (r < 0)
                 return r;
 
-        r = expand_names(bus, strv_skip(argv, 1), NULL, &names);
+        r = expand_names(bus, strv_skip(argv, 1), NULL, &names, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to expand names: %m");
 

test/TEST-41-ONESHOT-RESTART/Makefile

@@ -0,0 +1,9 @@
+BUILD_DIR=$(shell ../../tools/find-build-dir.sh)
+
+all setup run:
+	@basedir=../.. TEST_BASE_DIR=../ BUILD_DIR=$(BUILD_DIR) ./test.sh --$@
+
+clean clean-again:
+	@basedir=../.. TEST_BASE_DIR=../ BUILD_DIR=$(BUILD_DIR) ./test.sh --clean
+
+.PHONY: all setup run clean clean-again

test/TEST-41-ONESHOT-RESTART/test.sh

@@ -0,0 +1,32 @@
+#!/bin/bash
+set -e
+TEST_DESCRIPTION="Test oneshot unit restart on failure"
+. $TEST_BASE_DIR/test-functions
+
+test_setup() {
+    create_empty_image_rootdir
+
+    (
+        LOG_LEVEL=5
+        eval $(udevadm info --export --query=env --name=${LOOPDEV}p2)
+
+        setup_basic_environment
+        mask_supporting_services
+
+        # setup the testsuite service
+        cat >$initdir/etc/systemd/system/testsuite.service <<EOF
+[Unit]
+Description=Testsuite service
+
+[Service]
+ExecStart=/testsuite.sh
+Type=oneshot
+EOF
+        cp testsuite.sh $initdir/
+
+        setup_testsuite
+    )
+    setup_nspawn_root
+}
+
+do_test "$@"

test/TEST-41-ONESHOT-RESTART/testsuite.sh

@@ -0,0 +1,33 @@
+#!/bin/bash
+set -ex
+set -o pipefail
+
+systemd-analyze log-level debug
+systemd-analyze log-target console
+
+# These three commands should succeed.
+! systemd-run --unit=one -p Type=oneshot -p Restart=on-failure /bin/bash -c "exit 1"
+
+sleep 5
+
+if [[ "$(systemctl show one.service -p NRestarts --value)" -le 0 ]]; then
+  exit 1
+fi
+
+TMP_FILE="/test-41-oneshot-restart-test"
+
+touch $TMP_FILE
+
+! systemd-run --unit=two -p StartLimitBurst=3 -p Type=oneshot -p Restart=on-failure -p ExecStart="/bin/bash -c \"printf a >>  $TMP_FILE\"" /bin/bash -c "exit 1"
+
+sleep 5
+
+if [[ $(cat $TMP_FILE) != "aaa" ]]; then
+  exit 1
+fi
+
+systemd-analyze log-level info
+
+echo OK > /testok
+
+exit 0