Merge pull request #15547 from kkdwivedi/notify-barrier

Introduce sd_notify_barrier

9 files changed, 163 insertions, 12 deletions

diff --git a/man/rules/meson.build b/man/rules/meson.build
index 84f0442b1e..2096222c50 100644
--- a/man/rules/meson.build
+++ b/man/rules/meson.build
@@ -718,7 +718,7 @@ manpages = [
  ['sd_machine_get_class', '3', ['sd_machine_get_ifindices'], ''],
  ['sd_notify',
   '3',
-  ['sd_notifyf', 'sd_pid_notify', 'sd_pid_notify_with_fds', 'sd_pid_notifyf'],
+  ['sd_notifyf', 'sd_pid_notify', 'sd_pid_notify_with_fds', 'sd_pid_notifyf', 'sd_notify_barrier'],
   ''],
  ['sd_path_lookup', '3', ['sd_path_lookup_strv'], ''],
  ['sd_pid_get_owner_uid',
diff --git a/man/sd_notify.xml b/man/sd_notify.xml
index 0157ce864a..3e49386236 100644
--- a/man/sd_notify.xml
+++ b/man/sd_notify.xml
@@ -22,6 +22,7 @@
     <refname>sd_pid_notify</refname>
     <refname>sd_pid_notifyf</refname>
     <refname>sd_pid_notify_with_fds</refname>
+    <refname>sd_notify_barrier</refname>
     <refpurpose>Notify service manager about start-up completion and other service status changes</refpurpose>
   </refnamediv>
 
@@ -65,6 +66,12 @@
         <paramdef>const int *<parameter>fds</parameter></paramdef>
         <paramdef>unsigned <parameter>n_fds</parameter></paramdef>
       </funcprototype>
+
+      <funcprototype>
+        <funcdef>int <function>sd_notify_barrier</function></funcdef>
+        <paramdef>int <parameter>unset_environment</parameter></paramdef>
+        <paramdef>uint64_t <parameter>timeout</parameter></paramdef>
+      </funcprototype>
     </funcsynopsis>
   </refsynopsisdiv>
 
@@ -261,6 +268,17 @@
         as prematurely discarding file descriptors from the store.</para></listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term>BARRIER=1</term>
+
+        <listitem><para>Tells the service manager that the client is explicitly requesting synchronization by means of
+        closing the file descriptor sent with this command. The service manager gurantees that the processing of a <varname>
+        BARRIER=1</varname> command will only happen after all previous notification messages sent before this command
+        have been processed. Hence, this command accompanied with a single file descriptor can be used to synchronize
+        against reception of all previous status messages. Note that this command cannot be mixed with other notifications,
+        and has to be sent in a separate message to the service manager, otherwise all assignments will be ignored. Note that
+        sending 0 or more than 1 file descriptor with this command is a violation of the protocol.</para></listitem>
+      </varlistentry>
     </variablelist>
 
     <para>It is recommended to prefix variable names that are not
@@ -282,6 +300,13 @@
     attribute the message to the unit, and thus will ignore it, even if
     <varname>NotifyAccess=</varname><option>all</option> is set for it.</para>
 
+    <para>Hence, to eliminate all race conditions involving lookup of the client's unit and attribution of notifications
+    to units correctly, <function>sd_notify_barrier()</function> may be used. This call acts as a synchronization point
+    and ensures all notifications sent before this call have been picked up by the service manager when it returns
+    successfully. Use of <function>sd_notify_barrier()</function> is needed for clients which are not invoked by the
+    service manager, otherwise this synchronization mechanism is unnecessary for attribution of notifications to the
+    unit.</para>
+
     <para><function>sd_notifyf()</function> is similar to
     <function>sd_notify()</function> but takes a
     <function>printf()</function>-like format string plus
@@ -312,6 +337,14 @@
     to the service manager on messages that do not expect them (i.e.
     without <literal>FDSTORE=1</literal>) they are immediately closed
     on reception.</para>
+
+    <para><function>sd_notify_barrier()</function> allows the caller to
+    synchronize against reception of previously sent notification messages
+    and uses the <literal>BARRIER=1</literal> command. It takes a relative
+    <varname>timeout</varname> value in microseconds which is passed to
+    <citerefentry><refentrytitle>ppoll</refentrytitle><manvolnum>2</manvolnum>
+    </citerefentry>. A value of UINT64_MAX is interpreted as infinite timeout.
+    </para>
   </refsect1>
 
   <refsect1>
@@ -402,6 +435,22 @@
 
       <programlisting>sd_pid_notify_with_fds(0, 0, "FDSTORE=1\nFDNAME=foobar", &amp;fd, 1);</programlisting>
     </example>
+
+    <example>
+      <title>Eliminating race conditions</title>
+
+      <para>When the client sending the notifications is not spawned
+      by the service manager, it may exit too quickly and the service
+      manager may fail to attribute them correctly to the unit. To
+      prevent such races, use <function>sd_notify_barrier()</function>
+      to synchronize against reception of all notifications sent before
+      this call is made.</para>
+
+      <programlisting>sd_notify(0, "READY=1");
+      /* set timeout to 5 seconds */
+      sd_notify_barrier(0, 5 * 1000000);
+      </programlisting>
+    </example>
   </refsect1>
 
   <refsect1>
diff --git a/man/systemd-notify.xml b/man/systemd-notify.xml
index 4560074505..6d583003ba 100644
--- a/man/systemd-notify.xml
+++ b/man/systemd-notify.xml
@@ -54,15 +54,19 @@
     off the process, i.e. on all processes that match <varname>NotifyAccess=</varname><option>main</option> or
     <varname>NotifyAccess=</varname><option>exec</option>. Conversely, if an auxiliary process of the unit sends an
     <function>sd_notify()</function> message and immediately exits, the service manager might not be able to properly
-    attribute the message to the unit, and thus will ignore it, even if
-    <varname>NotifyAccess=</varname><option>all</option> is set for it.</para>
-
-    <para><command>systemd-notify</command> will first attempt to invoke <function>sd_notify()</function> pretending to
-    have the PID of the invoking process. This will only succeed when invoked with sufficient privileges. On failure,
-    it will then fall back to invoking it under its own PID. This behaviour is useful in order that when the tool is
-    invoked from a shell script the shell process — and not the <command>systemd-notify</command> process — appears as
-    sender of the message, which in turn is helpful if the shell process is the main process of a service, due to the
-    limitations of <varname>NotifyAccess=</varname><option>all</option> described above.</para>
+    attribute the message to the unit, and thus will ignore it, even if <varname>NotifyAccess=</varname><option>all
+    </option> is set for it. When <option>--no-block</option> is used, all synchronization for reception of notifications
+    is disabled, and hence the aforementioned race may occur if the invoking process is not the service manager or spawned
+    by the service manager.</para>
+
+    <para>Hence, <command>systemd-notify</command> will first attempt to invoke <function>sd_notify()</function>
+    pretending to have the PID of the invoking process. This will only succeed when invoked with sufficient privileges.
+    On failure, it will then fall back to invoking it under its own PID. This behaviour is useful in order that when
+    the tool is invoked from a shell script the shell process — and not the <command>systemd-notify</command> process
+    — appears as sender of the message, which in turn is helpful if the shell process is the main process of a service,
+    due to the limitations of <varname>NotifyAccess=</varname><option>all</option>. Use the <option>--pid=</option>
+    switch to tweak this behaviour.</para>
+
   </refsect1>
 
   <refsect1>
@@ -129,6 +133,17 @@
         with systemd.  </para></listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term><option>--no-block</option></term>
+
+        <listitem><para>Do not synchronously wait for the requested operation to finish.
+        Use of this option is only recommended when <command>systemd-notify</command>
+        is spawned by the service manager, or when the invoking process is directly spawned
+        by the service manager and has enough privileges to allow <command>systemd-notify
+        </command> to send the notification on its behalf. Sending notifications with
+        this option set is prone to race conditions in all other cases.</para></listitem>
+      </varlistentry>
+
       <xi:include href="standard-options.xml" xpointer="help" />
       <xi:include href="standard-options.xml" xpointer="version" />
     </variablelist>
diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index 398fd69b46..bba867f799 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -959,7 +959,14 @@
         <option>exec</option>. Conversely, if an auxiliary process of the unit sends an
         <function>sd_notify()</function> message and immediately exits, the service manager might not be able to
         properly attribute the message to the unit, and thus will ignore it, even if
-        <varname>NotifyAccess=</varname><option>all</option> is set for it.</para></listitem>
+        <varname>NotifyAccess=</varname><option>all</option> is set for it.</para>
+
+        <para>Hence, to eliminate all race conditions involving lookup of the client's unit and attribution of notifications
+        to units correctly, <function>sd_notify_barrier()</function> may be used. This call acts as a synchronization point
+        and ensures all notifications sent before this call have been picked up by the service manager when it returns
+        successfully. Use of <function>sd_notify_barrier()</function> is needed for clients which are not invoked by the
+        service manager, otherwise this synchronization mechanism is unnecessary for attribution of notifications to the
+        unit.</para></listitem>
       </varlistentry>
 
       <varlistentry>
diff --git a/man/systemd.xml b/man/systemd.xml
index 1534027d92..4e08ff6254 100644
--- a/man/systemd.xml
+++ b/man/systemd.xml
@@ -257,7 +257,7 @@
     execution compared to the target unit's state and is marked successful and
     complete when both satisfy. However, this job also pulls in other
     dependencies due to the defined relationships and thus leads to, in our
-    our example, start jobs for any of those inactive units getting queued as
+    example, start jobs for any of those inactive units getting queued as
     well.</para>
 
     <para>systemd contains native implementations of various tasks
diff --git a/src/core/manager.c b/src/core/manager.c
index 43b8a02e48..501e37339b 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -2284,6 +2284,20 @@ static int manager_dispatch_cgroups_agent_fd(sd_event_source *source, int fd, ui
         return 0;
 }
 
+static bool manager_process_barrier_fd(const char *buf, FDSet *fds) {
+        assert(buf);
+
+        /* nothing else must be sent when using BARRIER=1 */
+        if (STR_IN_SET(buf, "BARRIER=1", "BARRIER=1\n")) {
+                if (fdset_size(fds) != 1)
+                        log_warning("Got incorrect number of fds with BARRIER=1, closing them.");
+                return true;
+        } else if (startswith(buf, "BARRIER=1\n") || strstr(buf, "\nBARRIER=1\n") || endswith(buf, "\nBARRIER=1"))
+                log_warning("Extra notification messages sent with BARRIER=1, ignoring everything.");
+
+        return false;
+}
+
 static void manager_invoke_notify_message(
                 Manager *m,
                 Unit *u,
@@ -2417,6 +2431,10 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t
         /* Make sure it's NUL-terminated. */
         buf[n] = 0;
 
+        /* possibly a barrier fd, let's see */
+        if (manager_process_barrier_fd(buf, fds))
+                return 0;
+
         /* Increase the generation counter used for filtering out duplicate unit invocations. */
         m->notifygen++;
 
diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c
index 4cd71cb2d3..587a1f2595 100644
--- a/src/libsystemd/sd-daemon/sd-daemon.c
+++ b/src/libsystemd/sd-daemon/sd-daemon.c
@@ -4,6 +4,7 @@
 #include <limits.h>
 #include <mqueue.h>
 #include <netinet/in.h>
+#include <poll.h>
 #include <stdarg.h>
 #include <stddef.h>
 #include <stdio.h>
@@ -23,6 +24,7 @@
 #include "process-util.h"
 #include "socket-util.h"
 #include "strv.h"
+#include "time-util.h"
 #include "util.h"
 
 #define SNDBUF_SIZE (8*1024*1024)
@@ -551,6 +553,34 @@ finish:
         return r;
 }
 
+_public_ int sd_notify_barrier(int unset_environment, uint64_t timeout) {
+        _cleanup_close_pair_ int pipe_fd[2] = { -1, -1 };
+        struct timespec ts;
+        int r;
+
+        if (pipe2(pipe_fd, O_CLOEXEC) < 0)
+                return -errno;
+
+        r = sd_pid_notify_with_fds(0, unset_environment, "BARRIER=1", &pipe_fd[1], 1);
+        if (r <= 0)
+                return r;
+
+        pipe_fd[1] = safe_close(pipe_fd[1]);
+
+        struct pollfd pfd = {
+                .fd = pipe_fd[0],
+                /* POLLHUP is implicit */
+                .events = 0,
+        };
+        r = ppoll(&pfd, 1, timeout == UINT64_MAX ? NULL : timespec_store(&ts, timeout), NULL);
+        if (r < 0)
+                return -errno;
+        if (r == 0)
+                return -ETIMEDOUT;
+
+        return 1;
+}
+
 _public_ int sd_pid_notify(pid_t pid, int unset_environment, const char *state) {
         return sd_pid_notify_with_fds(pid, unset_environment, state, NULL, 0);
 }
diff --git a/src/notify/notify.c b/src/notify/notify.c
index 6c2dedd53f..69d473401d 100644
--- a/src/notify/notify.c
+++ b/src/notify/notify.c
@@ -18,6 +18,7 @@
 #include "string-util.h"
 #include "strv.h"
 #include "terminal-util.h"
+#include "time-util.h"
 #include "user-util.h"
 #include "util.h"
 
@@ -27,6 +28,7 @@ static const char *arg_status = NULL;
 static bool arg_booted = false;
 static uid_t arg_uid = UID_INVALID;
 static gid_t arg_gid = GID_INVALID;
+static bool arg_no_block = false;
 
 static int help(void) {
         _cleanup_free_ char *link = NULL;
@@ -45,6 +47,7 @@ static int help(void) {
                "     --uid=USER        Set user to send from\n"
                "     --status=TEXT     Set status text\n"
                "     --booted          Check if the system was booted up with systemd\n"
+               "     --no-block        Do not wait until operation finished\n"
                "\nSee the %s for details.\n"
                , program_invocation_short_name
                , ansi_highlight(), ansi_normal()
@@ -83,6 +86,7 @@ static int parse_argv(int argc, char *argv[]) {
                 ARG_STATUS,
                 ARG_BOOTED,
                 ARG_UID,
+                ARG_NO_BLOCK
         };
 
         static const struct option options[] = {
@@ -93,6 +97,7 @@ static int parse_argv(int argc, char *argv[]) {
                 { "status",    required_argument, NULL, ARG_STATUS    },
                 { "booted",    no_argument,       NULL, ARG_BOOTED    },
                 { "uid",       required_argument, NULL, ARG_UID       },
+                { "no-block",  no_argument,       NULL, ARG_NO_BLOCK  },
                 {}
         };
 
@@ -157,6 +162,10 @@ static int parse_argv(int argc, char *argv[]) {
                         break;
                 }
 
+                case ARG_NO_BLOCK:
+                        arg_no_block = true;
+                        break;
+
                 case '?':
                         return -EINVAL;
 
@@ -256,6 +265,16 @@ static int run(int argc, char* argv[]) {
         if (r == 0)
                 return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP),
                                        "No status data could be sent: $NOTIFY_SOCKET was not set");
+
+        if (!arg_no_block) {
+                r = sd_notify_barrier(0, 5 * USEC_PER_SEC);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to invoke barrier: %m");
+                if (r == 0)
+                        return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP),
+                                               "No status data could be sent: $NOTIFY_SOCKET was not set");
+        }
+
         return 0;
 }
 
diff --git a/src/systemd/sd-daemon.h b/src/systemd/sd-daemon.h
index 62b0f723c7..b47b15a445 100644
--- a/src/systemd/sd-daemon.h
+++ b/src/systemd/sd-daemon.h
@@ -287,6 +287,19 @@ int sd_pid_notifyf(pid_t pid, int unset_environment, const char *format, ...) _s
 int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char *state, const int *fds, unsigned n_fds);
 
 /*
+  Returns > 0 if synchronization with systemd succeeded.  Returns < 0
+  on error. Returns 0 if $NOTIFY_SOCKET was not set. Note that the
+  timeout parameter of this function call takes the timeout in µs, and
+  will be passed to ppoll(2), hence the behaviour will be similar to
+  ppoll(2). This function can be called after sending a status message
+  to systemd, if one needs to synchronize against reception of the
+  status messages sent before this call is made. Therefore, this
+  cannot be used to know if the status message was processed
+  successfully, but to only synchronize against its consumption.
+*/
+int sd_notify_barrier(int unset_environment, uint64_t timeout);
+
+/*
   Returns > 0 if the system was booted with systemd. Returns < 0 on
   error. Returns 0 if the system was not booted with systemd. Note
   that all of the functions above handle non-systemd boots just