pidref: several follow-ups for pidref_wait_for_terminate_full()

Follow-up for e74b571004661ff39fbcbddfe0cbf36d2fda0046

* Do not reuse si
* Refuse timeout == 0
* Use usec_add()
* Rename ret to ret_si - unlike others this one is not so obvious

diff --git a/src/basic/pidref.c b/src/basic/pidref.c
index de92d2fffbe6afd6eddc8b3fc7b361707923b5da..3c41e2a41ebfdd358e6291f5dd7f44b21411fea8 100644 (file)
--- a/src/basic/pidref.c
+++ b/src/basic/pidref.c
@@ -452,10 +452,11 @@ bool pidref_is_self(PidRef *pidref) {
         return pidref->fd_id == self_id;
 }
 
-int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret) {
-        siginfo_t si = {};
+int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret_si) {
         int r;
 
+        assert(timeout > 0);
+
         if (!pidref_is_set(pidref))
                 return -ESRCH;
 
@@ -468,10 +469,10 @@ int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *re
         if (timeout != USEC_INFINITY && pidref->fd < 0)
                 return -ENOMEDIUM;
 
-        usec_t ts = timeout == USEC_INFINITY ? USEC_INFINITY : now(CLOCK_MONOTONIC) + timeout;
+        usec_t ts = timeout == USEC_INFINITY ? USEC_INFINITY : usec_add(now(CLOCK_MONOTONIC), timeout);
 
         for (;;) {
-                if (timeout != USEC_INFINITY) {
+                if (ts != USEC_INFINITY) {
                         usec_t left = usec_sub_unsigned(ts, now(CLOCK_MONOTONIC));
                         if (left == 0)
                                 return -ETIMEDOUT;
@@ -485,22 +486,20 @@ int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *re
                                 return r;
                 }
 
+                siginfo_t si = {};
+
                 if (pidref->fd >= 0)
                         r = RET_NERRNO(waitid(P_PIDFD, pidref->fd, &si, WEXITED));
                 else
                         r = RET_NERRNO(waitid(P_PID, pidref->pid, &si, WEXITED));
-                if (r == -EINTR)
-                        continue;
-                if (r < 0)
+                if (r >= 0) {
+                        if (ret_si)
+                                *ret_si = si;
+                        return 0;
+                }
+                if (r != -EINTR)
                         return r;
-
-                break;
         }
-
-        if (ret)
-                *ret = si;
-
-        return 0;
 }
 
 bool pidref_is_automatic(const PidRef *pidref) {

diff --git a/src/basic/pidref.h b/src/basic/pidref.h
index 864acb569df91265288ff741c6888ce0b90129ac..2f1d95f9971d02c692e9e5b49fea4064152411b6 100644 (file)
--- a/src/basic/pidref.h
+++ b/src/basic/pidref.h
@@ -96,9 +96,9 @@ int pidref_kill(const PidRef *pidref, int sig);
 int pidref_kill_and_sigcont(const PidRef *pidref, int sig);
 int pidref_sigqueue(const PidRef *pidref, int sig, int value);
 
-int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret);
-static inline int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret) {
-        return pidref_wait_for_terminate_full(pidref, USEC_INFINITY, ret);
+int pidref_wait_for_terminate_full(PidRef *pidref, usec_t timeout, siginfo_t *ret_si);
+static inline int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret_si) {
+        return pidref_wait_for_terminate_full(pidref, USEC_INFINITY, ret_si);
 }
 
 static inline void pidref_done_sigterm_wait(PidRef *pidref) {

diff --git a/src/shutdown/shutdown.c b/src/shutdown/shutdown.c
index 3496ad33ea8d61bf025bd3460c2afb03c467b8f6..151c1654c24fb79e106ed82f80e38217040fa94f 100644 (file)
--- a/src/shutdown/shutdown.c
+++ b/src/shutdown/shutdown.c
@@ -250,7 +250,7 @@ int sync_with_progress(int fd) {
          * SYNC_PROGRESS_ATTEMPTS lapse without progress being made,
          * we assume that the sync is stalled */
         for (unsigned checks = 0; checks < SYNC_PROGRESS_ATTEMPTS; checks++) {
-                r = pidref_wait_for_terminate_full(&pidref, SYNC_TIMEOUT_USEC, /* ret= */ NULL);
+                r = pidref_wait_for_terminate_full(&pidref, SYNC_TIMEOUT_USEC, /* ret_si= */ NULL);
                 if (r == 0)
                         /* Sync finished without error (sync() call itself does not return an error code) */
                         return 0;

diff --git a/src/test/test-async.c b/src/test/test-async.c
index e65acfe6110c83300b26e628c6bbadb4c1dc59b2..46156d724dc979f0b416f048d2d906421bc06bd1 100644 (file)
--- a/src/test/test-async.c
+++ b/src/test/test-async.c
@@ -18,7 +18,7 @@
 TEST(asynchronous_sync) {
         _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
         ASSERT_OK(asynchronous_sync(&pidref));
-        ASSERT_OK(pidref_wait_for_terminate(&pidref, /* ret= */ NULL));
+        ASSERT_OK(pidref_wait_for_terminate(&pidref, NULL));
 }
 
 static void wait_fd_closed(int fd) {

diff --git a/src/test/test-namespace.c b/src/test/test-namespace.c
index 1c43c7ab10ea6da87cba546c8bb21fd65d37f528..4890ecc72958b2f779957f538c570e473d38b05f 100644 (file)
--- a/src/test/test-namespace.c
+++ b/src/test/test-namespace.c
@@ -326,7 +326,7 @@ TEST(process_is_owned_by_uid) {
         ASSERT_OK_ZERO(process_is_owned_by_uid(&pid, getuid()));
 
         ASSERT_OK(pidref_kill(&pid, SIGKILL));
-        ASSERT_OK(pidref_wait_for_terminate(&pid, /* ret= */ NULL));
+        ASSERT_OK(pidref_wait_for_terminate(&pid, NULL));
 
         /* Test a child that runs in a userns as uid 1, but the userns is owned by us */
         ASSERT_OK_ERRNO(pipe2(p, O_CLOEXEC));
@@ -373,7 +373,7 @@ TEST(process_is_owned_by_uid) {
         ASSERT_OK_POSITIVE(process_is_owned_by_uid(&pid, getuid()));
 
         ASSERT_OK(pidref_kill(&pid, SIGKILL));
-        ASSERT_OK(pidref_wait_for_terminate(&pid, /* ret= */ NULL));
+        ASSERT_OK(pidref_wait_for_terminate(&pid, NULL));
 }
 
 TEST(namespace_get_leader) {

diff --git a/src/test/test-pidref.c b/src/test/test-pidref.c
index f682b42d46e761c49d6f6cc46c2890d12d22e54d..eb4bfa223bd6cce0502a1a3b81d6b1967e045c04 100644 (file)
--- a/src/test/test-pidref.c
+++ b/src/test/test-pidref.c
@@ -243,7 +243,7 @@ TEST(pidref_is_remote) {
         ASSERT_FALSE(pidref_is_automatic(&p));
         ASSERT_ERROR(pidref_kill(&p, SIGTERM), EREMOTE);
         ASSERT_ERROR(pidref_kill_and_sigcont(&p, SIGTERM), EREMOTE);
-        ASSERT_ERROR(pidref_wait_for_terminate(&p, /* ret= */ NULL), EREMOTE);
+        ASSERT_ERROR(pidref_wait_for_terminate(&p, NULL), EREMOTE);
         ASSERT_ERROR(pidref_verify(&p), EREMOTE);
 }