Merge pull request #21216 from poettering/take-fd-tweak

fd-util: make TAKE_FD free of double evaluation

src/basic/fd-util.h

@@ -91,9 +91,10 @@ static inline int make_null_stdio(void) {
 /* Like TAKE_PTR() but for file descriptors, resetting them to -1 */
 #define TAKE_FD(fd)                             \
         ({                                      \
-                int _fd_ = (fd);                \
-                (fd) = -1;                      \
-                _fd_;                           \
+                int *_fd_ = &(fd);              \
+                int _ret_ = *_fd_;              \
+                *_fd_ = -1;                     \
+                _ret_;                          \
         })
 
 /* Like free_and_replace(), but for file descriptors */

src/basic/process-util.c

@@ -857,8 +857,8 @@ int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout) {
 void sigkill_wait(pid_t pid) {
         assert(pid > 1);
 
-        if (kill(pid, SIGKILL) >= 0)
-                (void) wait_for_terminate(pid, NULL);
+        (void) kill(pid, SIGKILL);
+        (void) wait_for_terminate(pid, NULL);
 }
 
 void sigkill_waitp(pid_t *pid) {
@@ -875,8 +875,8 @@ void sigkill_waitp(pid_t *pid) {
 void sigterm_wait(pid_t pid) {
         assert(pid > 1);
 
-        if (kill_and_sigcont(pid, SIGTERM) >= 0)
-                (void) wait_for_terminate(pid, NULL);
+        (void) kill_and_sigcont(pid, SIGTERM);
+        (void) wait_for_terminate(pid, NULL);
 }
 
 int kill_and_sigcont(pid_t pid, int sig) {

src/basic/process-util.h

@@ -192,8 +192,9 @@ assert_cc(TASKS_MAX <= (unsigned long) PID_T_MAX);
 /* Like TAKE_PTR() but for child PIDs, resetting them to 0 */
 #define TAKE_PID(pid)                           \
         ({                                      \
-                pid_t _pid_ = (pid);            \
-                (pid) = 0;                      \
+                pid_t *_ppid_ = &(pid);         \
+                pid_t _pid_ = *_ppid_;          \
+                *_ppid_ = 0;                    \
                 _pid_;                          \
         })
 

src/core/execute.c

@@ -2248,8 +2248,7 @@ static int setup_private_users(uid_t ouid, gid_t ogid, uid_t uid, gid_t gid) {
         if (n != 0) /* on success we should have read 0 bytes */
                 return -EIO;
 
-        r = wait_for_terminate_and_check("(sd-userns)", pid, 0);
-        pid = 0;
+        r = wait_for_terminate_and_check("(sd-userns)", TAKE_PID(pid), 0);
         if (r < 0)
                 return r;
         if (r != EXIT_SUCCESS) /* If something strange happened with the child, let's consider this fatal, too */

src/core/mount.c

@@ -236,8 +236,7 @@ static void mount_unwatch_control_pid(Mount *m) {
         if (m->control_pid <= 0)
                 return;
 
-        unit_unwatch_pid(UNIT(m), m->control_pid);
-        m->control_pid = 0;
+        unit_unwatch_pid(UNIT(m), TAKE_PID(m->control_pid));
 }
 
 static void mount_parameters_done(MountParameters *p) {

src/core/service.c

@@ -130,8 +130,7 @@ static void service_unwatch_control_pid(Service *s) {
         if (s->control_pid <= 0)
                 return;
 
-        unit_unwatch_pid(UNIT(s), s->control_pid);
-        s->control_pid = 0;
+        unit_unwatch_pid(UNIT(s), TAKE_PID(s->control_pid));
 }
 
 static void service_unwatch_main_pid(Service *s) {
@@ -140,8 +139,7 @@ static void service_unwatch_main_pid(Service *s) {
         if (s->main_pid <= 0)
                 return;
 
-        unit_unwatch_pid(UNIT(s), s->main_pid);
-        s->main_pid = 0;
+        unit_unwatch_pid(UNIT(s), TAKE_PID(s->main_pid));
 }
 
 static void service_unwatch_pid_file(Service *s) {

src/core/socket.c

@@ -109,8 +109,7 @@ static void socket_unwatch_control_pid(Socket *s) {
         if (s->control_pid <= 0)
                 return;
 
-        unit_unwatch_pid(UNIT(s), s->control_pid);
-        s->control_pid = 0;
+        unit_unwatch_pid(UNIT(s), TAKE_PID(s->control_pid));
 }
 
 static void socket_cleanup_fd_list(SocketPort *p) {

src/core/swap.c

@@ -155,8 +155,7 @@ static void swap_unwatch_control_pid(Swap *s) {
         if (s->control_pid <= 0)
                 return;
 
-        unit_unwatch_pid(UNIT(s), s->control_pid);
-        s->control_pid = 0;
+        unit_unwatch_pid(UNIT(s), TAKE_PID(s->control_pid));
 }
 
 static void swap_done(Unit *u) {

src/fundamental/macro-fundamental.h

@@ -257,8 +257,9 @@
  * resets it to NULL. See: https://doc.rust-lang.org/std/option/enum.Option.html#method.take */
 #define TAKE_PTR(ptr)                           \
         ({                                      \
-                typeof(ptr) _ptr_ = (ptr);      \
-                (ptr) = NULL;                   \
+                typeof(ptr) *_pptr_ = &(ptr);   \
+                typeof(ptr) _ptr_ = *_pptr_;    \
+                *_pptr_ = NULL;                 \
                 _ptr_;                          \
         })
 

src/import/export-tar.c

@@ -56,10 +56,8 @@ TarExport *tar_export_unref(TarExport *e) {
 
         sd_event_source_unref(e->output_event_source);
 
-        if (e->tar_pid > 1) {
-                (void) kill_and_sigcont(e->tar_pid, SIGKILL);
-                (void) wait_for_terminate(e->tar_pid, NULL);
-        }
+        if (e->tar_pid > 1)
+                sigkill_wait(e->tar_pid);
 
         if (e->temp_path) {
                 (void) btrfs_subvol_remove(e->temp_path, BTRFS_REMOVE_QUOTA);
@@ -147,8 +145,7 @@ static int tar_export_finish(TarExport *e) {
         assert(e->tar_fd >= 0);
 
         if (e->tar_pid > 0) {
-                r = wait_for_terminate_and_check("tar", e->tar_pid, WAIT_LOG);
-                e->tar_pid = 0;
+                r = wait_for_terminate_and_check("tar", TAKE_PID(e->tar_pid), WAIT_LOG);
                 if (r < 0)
                         return r;
                 if (r != EXIT_SUCCESS)

src/import/import-tar.c

@@ -69,10 +69,8 @@ TarImport* tar_import_unref(TarImport *i) {
 
         sd_event_source_unref(i->input_event_source);
 
-        if (i->tar_pid > 1) {
-                (void) kill_and_sigcont(i->tar_pid, SIGKILL);
-                (void) wait_for_terminate(i->tar_pid, NULL);
-        }
+        if (i->tar_pid > 1)
+                sigkill_wait(i->tar_pid);
 
         rm_rf_subvolume_and_free(i->temp_path);
 
@@ -167,8 +165,7 @@ static int tar_import_finish(TarImport *i) {
         i->tar_fd = safe_close(i->tar_fd);
 
         if (i->tar_pid > 0) {
-                r = wait_for_terminate_and_check("tar", i->tar_pid, WAIT_LOG);
-                i->tar_pid = 0;
+                r = wait_for_terminate_and_check("tar", TAKE_PID(i->tar_pid), WAIT_LOG);
                 if (r < 0)
                         return r;
                 if (r != EXIT_SUCCESS)

src/import/importd.c

@@ -126,10 +126,8 @@ static Transfer *transfer_unref(Transfer *t) {
         free(t->format);
         free(t->object_path);
 
-        if (t->pid > 0) {
-                (void) kill_and_sigcont(t->pid, SIGKILL);
-                (void) wait_for_terminate(t->pid, NULL);
-        }
+        if (t->pid > 1)
+                sigkill_wait(t->pid);
 
         safe_close(t->log_fd);
         safe_close(t->stdin_fd);

src/import/pull-common.c

@@ -482,8 +482,7 @@ static int verify_gpg(
 
         gpg_pipe[1] = safe_close(gpg_pipe[1]);
 
-        r = wait_for_terminate_and_check("gpg", pid, WAIT_LOG_ABNORMAL);
-        pid = 0;
+        r = wait_for_terminate_and_check("gpg", TAKE_PID(pid), WAIT_LOG_ABNORMAL);
         if (r < 0)
                 goto finish;
         if (r != EXIT_SUCCESS)

src/import/pull-tar.c

@@ -71,10 +71,8 @@ TarPull* tar_pull_unref(TarPull *i) {
         if (!i)
                 return NULL;
 
-        if (i->tar_pid > 1) {
-                (void) kill_and_sigcont(i->tar_pid, SIGKILL);
-                (void) wait_for_terminate(i->tar_pid, NULL);
-        }
+        if (i->tar_pid > 1)
+                sigkill_wait(i->tar_pid);
 
         pull_job_unref(i->tar_job);
         pull_job_unref(i->checksum_job);
@@ -369,8 +367,7 @@ static void tar_pull_job_on_finished(PullJob *j) {
         pull_job_close_disk_fd(i->settings_job);
 
         if (i->tar_pid > 0) {
-                r = wait_for_terminate_and_check("tar", i->tar_pid, WAIT_LOG);
-                i->tar_pid = 0;
+                r = wait_for_terminate_and_check("tar", TAKE_PID(i->tar_pid), WAIT_LOG);
                 if (r < 0)
                         goto finish;
                 if (r != EXIT_SUCCESS) {

src/libsystemd/sd-bus/sd-bus.c

@@ -1082,10 +1082,10 @@ static int bus_parse_next_address(sd_bus *b) {
 }
 
 static void bus_kill_exec(sd_bus *bus) {
-        if (pid_is_valid(bus->busexec_pid) > 0) {
-                sigterm_wait(bus->busexec_pid);
-                bus->busexec_pid = 0;
-        }
+        if (!pid_is_valid(bus->busexec_pid))
+                return;
+
+        sigterm_wait(TAKE_PID(bus->busexec_pid));
 }
 
 static int bus_start_address(sd_bus *b) {

src/nspawn/nspawn.c

@@ -5248,8 +5248,7 @@ static int run_container(
                 }
         }
 
-        r = wait_for_container(*pid, &container_status);
-        *pid = 0;
+        r = wait_for_container(TAKE_PID(*pid), &container_status);
 
         /* Tell machined that we are gone. */
         if (bus)

src/shared/pager.c

@@ -286,7 +286,7 @@ void pager_close(void) {
         stdout_redirected = stderr_redirected = false;
 
         (void) kill(pager_pid, SIGCONT);
-        (void) wait_for_terminate(pager_pid, NULL);
+        (void) wait_for_terminate(TAKE_PID(pager_pid), NULL);
         pager_pid = 0;
 }
 

src/shared/spawn-ask-password-agent.c

@@ -43,9 +43,7 @@ void ask_password_agent_close(void) {
                 return;
 
         /* Inform agent that we are done */
-        (void) kill_and_sigcont(agent_pid, SIGTERM);
-        (void) wait_for_terminate(agent_pid, NULL);
-        agent_pid = 0;
+        sigterm_wait(TAKE_PID(agent_pid));
 }
 
 int ask_password_agent_open_if_enabled(BusTransport transport, bool ask_password) {

src/shared/spawn-polkit-agent.c

@@ -69,9 +69,7 @@ void polkit_agent_close(void) {
                 return;
 
         /* Inform agent that we are done */
-        (void) kill_and_sigcont(agent_pid, SIGTERM);
-        (void) wait_for_terminate(agent_pid, NULL);
-        agent_pid = 0;
+        sigterm_wait(TAKE_PID(agent_pid));
 }
 
 #else

src/test/test-fd-util.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
 #include <fcntl.h>
+#include <sys/eventfd.h>
 #include <unistd.h>
 
 #include "alloc-util.h"
@@ -486,6 +487,47 @@ static void test_fd_reopen(void) {
         fd1 = -1;
 }
 
+static void test_take_fd(void) {
+        _cleanup_close_ int fd1 = -1, fd2 = -1;
+        int array[2] = { -1, -1 }, i = 0;
+
+        log_info("/* %s */", __func__);
+
+        assert_se(fd1 == -1);
+        assert_se(fd2 == -1);
+
+        fd1 = eventfd(0, EFD_CLOEXEC);
+        assert_se(fd1 >= 0);
+
+        fd2 = TAKE_FD(fd1);
+        assert_se(fd1 == -1);
+        assert_se(fd2 >= 0);
+
+        assert_se(array[0] == -1);
+        assert_se(array[1] == -1);
+
+        array[0] = TAKE_FD(fd2);
+        assert_se(fd1 == -1);
+        assert_se(fd2 == -1);
+        assert_se(array[0] >= 0);
+        assert_se(array[1] == -1);
+
+        array[1] = TAKE_FD(array[i]);
+        assert_se(array[0] == -1);
+        assert_se(array[1] >= 0);
+
+        i = 1 - i;
+        array[0] = TAKE_FD(*(array + i));
+        assert_se(array[0] >= 0);
+        assert_se(array[1] == -1);
+
+        i = 1 - i;
+        fd1 = TAKE_FD(array[i]);
+        assert_se(fd1 >= 0);
+        assert_se(array[0] == -1);
+        assert_se(array[1] == -1);
+}
+
 int main(int argc, char *argv[]) {
 
         test_setup_logging(LOG_DEBUG);
@@ -500,6 +542,7 @@ int main(int argc, char *argv[]) {
         test_close_all_fds();
         test_format_proc_fd_path();
         test_fd_reopen();
+        test_take_fd();
 
         return 0;
 }