From 495e75ed5c8cce933947dae10a4a1b5f8067e432 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Sep 2023 21:58:55 +0200 Subject: [PATCH] core: move pid watch/unwatch logic of the service manager to pidfd This makes sure unit_watch_pid() and unit_unwatch_pid() will track processes by pidfd if supported. Also ports over some related code. Should not really change behaviour. Note that this does *not* add support waiting for POLLIN on the pidfds as additional exit notification. This is left for a later commit (this commit is already large enough), in particular as that would add new logic and not just convert existing logic. --- src/core/cgroup.c | 168 ++++++++++++++++++++++------------- src/core/cgroup.h | 7 +- src/core/dbus-manager.c | 12 +-- src/core/dbus-scope.c | 9 +- src/core/dbus-unit.c | 20 ++--- src/core/manager.c | 34 ++++--- src/core/manager.h | 19 ++-- src/core/mount.c | 6 +- src/core/scope.c | 27 +++--- src/core/service.c | 84 +++++++++--------- src/core/socket.c | 8 +- src/core/swap.c | 6 +- src/core/unit.c | 190 +++++++++++++++++++++++++--------------- src/core/unit.h | 12 +-- 14 files changed, 354 insertions(+), 248 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index e8f8ddc2445..18be0650450 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2499,7 +2499,7 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { _cleanup_free_ char *joined = NULL; CGroupMask delegated_mask; const char *p; - void *pidp; + PidRef *pid; int ret, r; assert(u); @@ -2533,17 +2533,25 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { delegated_mask = unit_get_delegate_mask(u); ret = 0; - SET_FOREACH(pidp, pids) { - pid_t pid = PTR_TO_PID(pidp); + SET_FOREACH(pid, pids) { + + /* Unfortunately we cannot add pids by pidfd to a cgroup. Hence we have to use PIDs instead, + * which of course is racy. Let's shorten the race a bit though, and re-validate the PID + * before we use it */ + r = pidref_verify(pid); + if (r < 0) { + log_unit_info_errno(u, r, "PID " PID_FMT " vanished before we could move it to target cgroup '%s', skipping: %m", pid->pid, empty_to_root(p)); + continue; + } /* First, attach the PID to the main cgroup hierarchy */ - r = cg_attach(SYSTEMD_CGROUP_CONTROLLER, p, pid); + r = cg_attach(SYSTEMD_CGROUP_CONTROLLER, p, pid->pid); if (r < 0) { bool again = MANAGER_IS_USER(u->manager) && ERRNO_IS_PRIVILEGE(r); log_unit_full_errno(u, again ? LOG_DEBUG : LOG_INFO, r, "Couldn't move process "PID_FMT" to%s requested cgroup '%s': %m", - pid, again ? " directly" : "", empty_to_root(p)); + pid->pid, again ? " directly" : "", empty_to_root(p)); if (again) { int z; @@ -2553,9 +2561,9 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { * Since it's more privileged it might be able to move the process across the * leaves of a subtree whose top node is not owned by us. */ - z = unit_attach_pid_to_cgroup_via_bus(u, pid, suffix_path); + z = unit_attach_pid_to_cgroup_via_bus(u, pid->pid, suffix_path); if (z < 0) - log_unit_info_errno(u, z, "Couldn't move process "PID_FMT" to requested cgroup '%s' (directly or via the system bus): %m", pid, empty_to_root(p)); + log_unit_info_errno(u, z, "Couldn't move process "PID_FMT" to requested cgroup '%s' (directly or via the system bus): %m", pid->pid, empty_to_root(p)); else { if (ret >= 0) ret++; /* Count successful additions */ @@ -2588,12 +2596,12 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { /* If this controller is delegated and realized, honour the caller's request for the cgroup suffix. */ if (delegated_mask & u->cgroup_realized_mask & bit) { - r = cg_attach(cgroup_controller_to_string(c), p, pid); + r = cg_attach(cgroup_controller_to_string(c), p, pid->pid); if (r >= 0) continue; /* Success! */ log_unit_debug_errno(u, r, "Failed to attach PID " PID_FMT " to requested cgroup %s in controller %s, falling back to unit's cgroup: %m", - pid, empty_to_root(p), cgroup_controller_to_string(c)); + pid->pid, empty_to_root(p), cgroup_controller_to_string(c)); } /* So this controller is either not delegate or realized, or something else weird happened. In @@ -2603,10 +2611,10 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { if (!realized) continue; /* Not even realized in the root slice? Then let's not bother */ - r = cg_attach(cgroup_controller_to_string(c), realized, pid); + r = cg_attach(cgroup_controller_to_string(c), realized, pid->pid); if (r < 0) log_unit_debug_errno(u, r, "Failed to attach PID " PID_FMT " to realized cgroup %s in controller %s, ignoring: %m", - pid, realized, cgroup_controller_to_string(c)); + pid->pid, realized, cgroup_controller_to_string(c)); } } @@ -3049,9 +3057,9 @@ void unit_prune_cgroup(Unit *u) { u->bpf_device_control_installed = bpf_program_free(u->bpf_device_control_installed); } -int unit_search_main_pid(Unit *u, pid_t *ret) { +int unit_search_main_pid(Unit *u, PidRef *ret) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; _cleanup_fclose_ FILE *f = NULL; - pid_t pid = 0, npid; int r; assert(u); @@ -3064,25 +3072,33 @@ int unit_search_main_pid(Unit *u, pid_t *ret) { if (r < 0) return r; - while (cg_read_pid(f, &npid) > 0) { + for (;;) { + _cleanup_(pidref_done) PidRef npidref = PIDREF_NULL; - if (npid == pid) - continue; + r = cg_read_pidref(f, &npidref); + if (r < 0) + return r; + if (r == 0) + break; - if (pid_is_my_child(npid) == 0) + if (pidref_equal(&pidref, &npidref)) /* seen already, cgroupfs reports duplicates! */ continue; - if (pid != 0) - /* Dang, there's more than one daemonized PID - in this group, so we don't know what process - is the main process. */ + if (pid_is_my_child(npidref.pid) == 0) /* ignore processes further down the tree */ + continue; + if (pidref_is_set(&pidref) != 0) + /* Dang, there's more than one daemonized PID in this group, so we don't know what + * process is the main process. */ return -ENODATA; - pid = npid; + pidref = TAKE_PIDREF(npidref); } - *ret = pid; + if (!pidref_is_set(&pidref)) + return -ENODATA; + + *ret = TAKE_PIDREF(pidref); return 0; } @@ -3096,43 +3112,44 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) { r = cg_enumerate_processes(SYSTEMD_CGROUP_CONTROLLER, path, &f); if (r < 0) - ret = r; + RET_GATHER(ret, r); else { - pid_t pid; + for (;;) { + _cleanup_(pidref_done) PidRef pid = PIDREF_NULL; - while ((r = cg_read_pid(f, &pid)) > 0) { - r = unit_watch_pid(u, pid, false); - if (r < 0 && ret >= 0) - ret = r; - } + r = cg_read_pidref(f, &pid); + if (r == 0) + break; + if (r < 0) { + RET_GATHER(ret, r); + break; + } - if (r < 0 && ret >= 0) - ret = r; + RET_GATHER(ret, unit_watch_pidref(u, &pid, /* exclusive= */ false)); + } } r = cg_enumerate_subgroups(SYSTEMD_CGROUP_CONTROLLER, path, &d); - if (r < 0) { - if (ret >= 0) - ret = r; - } else { - char *fn; + if (r < 0) + RET_GATHER(ret, r); + else { + for (;;) { + _cleanup_free_ char *fn = NULL, *p = NULL; - while ((r = cg_read_subgroup(d, &fn)) > 0) { - _cleanup_free_ char *p = NULL; + r = cg_read_subgroup(d, &fn); + if (r == 0) + break; + if (r < 0) { + RET_GATHER(ret, r); + break; + } p = path_join(empty_to_root(path), fn); - free(fn); - if (!p) return -ENOMEM; - r = unit_watch_pids_in_path(u, p); - if (r < 0 && ret >= 0) - ret = r; + RET_GATHER(ret, unit_watch_pids_in_path(u, p)); } - - if (r < 0 && ret >= 0) - ret = r; } return ret; @@ -3762,50 +3779,77 @@ Unit* manager_get_unit_by_cgroup(Manager *m, const char *cgroup) { } } -Unit *manager_get_unit_by_pid_cgroup(Manager *m, pid_t pid) { +Unit *manager_get_unit_by_pidref_cgroup(Manager *m, PidRef *pid) { _cleanup_free_ char *cgroup = NULL; assert(m); - if (!pid_is_valid(pid)) + if (!pidref_is_set(pid)) return NULL; - if (cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid, &cgroup) < 0) + if (cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid->pid, &cgroup) < 0) return NULL; return manager_get_unit_by_cgroup(m, cgroup); } -Unit *manager_get_unit_by_pid(Manager *m, pid_t pid) { +Unit *manager_get_unit_by_pidref_watching(Manager *m, PidRef *pid) { Unit *u, **array; assert(m); - /* Note that a process might be owned by multiple units, we return only one here, which is good enough for most - * cases, though not strictly correct. We prefer the one reported by cgroup membership, as that's the most - * relevant one as children of the process will be assigned to that one, too, before all else. */ + if (!pidref_is_set(pid)) + return NULL; - if (!pid_is_valid(pid)) + u = hashmap_get(m->watch_pids, pid); + if (u) + return u; + + array = hashmap_get(m->watch_pids_more, pid); + if (array) + return array[0]; + + return NULL; +} + +Unit *manager_get_unit_by_pidref(Manager *m, PidRef *pid) { + Unit *u; + + assert(m); + + /* Note that a process might be owned by multiple units, we return only one here, which is good + * enough for most cases, though not strictly correct. We prefer the one reported by cgroup + * membership, as that's the most relevant one as children of the process will be assigned to that + * one, too, before all else. */ + + if (!pidref_is_set(pid)) return NULL; - if (pid == getpid_cached()) + if (pid->pid == getpid_cached()) return hashmap_get(m->units, SPECIAL_INIT_SCOPE); + if (pid->pid == 1) + return NULL; - u = manager_get_unit_by_pid_cgroup(m, pid); + u = manager_get_unit_by_pidref_cgroup(m, pid); if (u) return u; - u = hashmap_get(m->watch_pids, PID_TO_PTR(pid)); + u = manager_get_unit_by_pidref_watching(m, pid); if (u) return u; - array = hashmap_get(m->watch_pids, PID_TO_PTR(-pid)); - if (array) - return array[0]; - return NULL; } +Unit *manager_get_unit_by_pid(Manager *m, pid_t pid) { + assert(m); + + if (!pid_is_valid(pid)) + return NULL; + + return manager_get_unit_by_pidref(m, &PIDREF_MAKE_FROM_PID(pid)); +} + int manager_notify_cgroup_empty(Manager *m, const char *cgroup) { Unit *u; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 90159fd84cc..05dfd482774 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -8,6 +8,7 @@ #include "cpu-set-util.h" #include "firewall-util.h" #include "list.h" +#include "pidref.h" #include "time-util.h" typedef struct TasksMax { @@ -323,14 +324,16 @@ void manager_shutdown_cgroup(Manager *m, bool delete); unsigned manager_dispatch_cgroup_realize_queue(Manager *m); Unit *manager_get_unit_by_cgroup(Manager *m, const char *cgroup); -Unit *manager_get_unit_by_pid_cgroup(Manager *m, pid_t pid); +Unit *manager_get_unit_by_pidref_cgroup(Manager *m, PidRef *pid); +Unit *manager_get_unit_by_pidref_watching(Manager *m, PidRef *pid); +Unit* manager_get_unit_by_pidref(Manager *m, PidRef *pid); Unit* manager_get_unit_by_pid(Manager *m, pid_t pid); uint64_t unit_get_ancestor_memory_min(Unit *u); uint64_t unit_get_ancestor_memory_low(Unit *u); uint64_t unit_get_ancestor_startup_memory_low(Unit *u); -int unit_search_main_pid(Unit *u, pid_t *ret); +int unit_search_main_pid(Unit *u, PidRef *ret); int unit_watch_all_pids(Unit *u); int unit_synthesize_cgroup_empty_event(Unit *u); diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 15ccfdc2d3d..9917419096b 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -679,10 +679,10 @@ static int method_get_unit_by_control_group(sd_bus_message *message, void *userd static int method_get_unit_by_pidfd(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; Manager *m = ASSERT_PTR(userdata); _cleanup_free_ char *path = NULL; int r, pidfd; - pid_t pid; Unit *u; assert(message); @@ -691,13 +691,13 @@ static int method_get_unit_by_pidfd(sd_bus_message *message, void *userdata, sd_ if (r < 0) return r; - r = pidfd_get_pid(pidfd, &pid); + r = pidref_set_pidfd(&pidref, pidfd); if (r < 0) return sd_bus_error_set_errnof(error, r, "Failed to get PID from PIDFD: %m"); - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) - return sd_bus_error_setf(error, BUS_ERROR_NO_UNIT_FOR_PID, "PID "PID_FMT" does not belong to any loaded unit.", pid); + return sd_bus_error_setf(error, BUS_ERROR_NO_UNIT_FOR_PID, "PID "PID_FMT" does not belong to any loaded unit.", pidref.pid); r = mac_selinux_unit_access_check(u, message, "status", error); if (r < 0) @@ -721,12 +721,12 @@ static int method_get_unit_by_pidfd(sd_bus_message *message, void *userdata, sd_ /* Double-check that the process is still alive and that the PID did not change before returning the * answer. */ - r = pidfd_verify_pid(pidfd, pid); + r = pidref_verify(&pidref); if (r == -ESRCH) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_PROCESS, "The PIDFD's PID "PID_FMT" changed during the lookup operation.", - pid); + pidref.pid); if (r < 0) return sd_bus_error_set_errnof(error, r, "Failed to get PID from PIDFD: %m"); diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index 7b07bb8bb9c..97a8d277fb6 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -92,6 +92,7 @@ static int bus_scope_set_transient_property( return r; for (;;) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; uint32_t upid; pid_t pid; @@ -114,12 +115,16 @@ static int bus_scope_set_transient_property( } else pid = (uid_t) upid; - r = unit_pid_attachable(u, pid, error); + r = pidref_set_pid(&pidref, pid); + if (r < 0) + return r; + + r = unit_pid_attachable(u, &pidref, error); if (r < 0) return r; if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_watch_pid(u, pid, false); + r = unit_watch_pidref(u, &pidref, /* exclusive= */ false); if (r < 0 && r != -EEXIST) return r; } diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 05b80cbf331..354fa5290be 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1439,7 +1439,6 @@ static int property_get_io_counter( } int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd_bus_error *error) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; _cleanup_set_free_ Set *pids = NULL; Unit *u = userdata; @@ -1484,6 +1483,7 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd if (r < 0) return r; for (;;) { + _cleanup_(pidref_freep) PidRef *pidref = NULL; uid_t process_uid, sender_uid; uint32_t upid; pid_t pid; @@ -1501,12 +1501,16 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd } else pid = (uid_t) upid; + r = pidref_new_from_pid(pid, &pidref); + if (r < 0) + return r; + /* Filter out duplicates */ - if (set_contains(pids, PID_TO_PTR(pid))) + if (set_contains(pids, pidref)) continue; /* Check if this process is suitable for attaching to this unit */ - r = unit_pid_attachable(u, pid, error); + r = unit_pid_attachable(u, pidref, error); if (r < 0) return r; @@ -1518,7 +1522,7 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd /* Let's validate security: if the sender is root, then all is OK. If the sender is any other unit, * then the process' UID and the target unit's UID have to match the sender's UID */ if (sender_uid != 0 && sender_uid != getuid()) { - r = get_process_uid(pid, &process_uid); + r = get_process_uid(pidref->pid, &process_uid); if (r < 0) return sd_bus_error_set_errnof(error, r, "Failed to retrieve process UID: %m"); @@ -1528,13 +1532,7 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by target unit's UID. Refusing.", pid); } - if (!pids) { - pids = set_new(NULL); - if (!pids) - return -ENOMEM; - } - - r = set_put(pids, PID_TO_PTR(pid)); + r = set_ensure_consume(&pids, &pidref_hash_ops, TAKE_PTR(pidref)); if (r < 0) return r; } diff --git a/src/core/manager.c b/src/core/manager.c index cba3aae9ff9..07a95fe752d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1622,6 +1622,7 @@ Manager* manager_free(Manager *m) { hashmap_free(m->units_by_invocation_id); hashmap_free(m->jobs); hashmap_free(m->watch_pids); + hashmap_free(m->watch_pids_more); hashmap_free(m->watch_bus); prioq_free(m->run_queue); @@ -2374,14 +2375,18 @@ void manager_clear_jobs(Manager *m) { job_finish_and_invalidate(j, JOB_CANCELED, false, false); } -void manager_unwatch_pid(Manager *m, pid_t pid) { +void manager_unwatch_pidref(Manager *m, PidRef *pid) { assert(m); - /* First let's drop the unit keyed as "pid". */ - (void) hashmap_remove(m->watch_pids, PID_TO_PTR(pid)); + for (;;) { + Unit *u; - /* Then, let's also drop the array keyed by -pid. */ - free(hashmap_remove(m->watch_pids, PID_TO_PTR(-pid))); + u = manager_get_unit_by_pidref_watching(m, pid); + if (!u) + break; + + unit_unwatch_pidref(u, pid); + } } static int manager_dispatch_run_queue(sd_event_source *source, void *userdata) { @@ -2674,10 +2679,13 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t /* Increase the generation counter used for filtering out duplicate unit invocations. */ m->notifygen++; + /* Generate lookup key from the PID (we have no pidfd here, after all) */ + PidRef pidref = PIDREF_MAKE_FROM_PID(ucred->pid); + /* Notify every unit that might be interested, which might be multiple. */ - u1 = manager_get_unit_by_pid_cgroup(m, ucred->pid); - u2 = hashmap_get(m->watch_pids, PID_TO_PTR(ucred->pid)); - array = hashmap_get(m->watch_pids, PID_TO_PTR(-ucred->pid)); + u1 = manager_get_unit_by_pidref_cgroup(m, &pidref); + u2 = hashmap_get(m->watch_pids, &pidref); + array = hashmap_get(m->watch_pids_more, &pidref); if (array) { size_t k = 0; @@ -2773,10 +2781,14 @@ static int manager_dispatch_sigchld(sd_event_source *source, void *userdata) { /* Increase the generation counter used for filtering out duplicate unit invocations */ m->sigchldgen++; + /* We look this up by a PidRef that only consists of the PID. After all we couldn't create a + * pidfd here any more even if we wanted (since the process just exited). */ + PidRef pidref = PIDREF_MAKE_FROM_PID(si.si_pid); + /* And now figure out the unit this belongs to, it might be multiple... */ - u1 = manager_get_unit_by_pid_cgroup(m, si.si_pid); - u2 = hashmap_get(m->watch_pids, PID_TO_PTR(si.si_pid)); - array = hashmap_get(m->watch_pids, PID_TO_PTR(-si.si_pid)); + u1 = manager_get_unit_by_pidref_cgroup(m, &pidref); + u2 = hashmap_get(m->watch_pids, &pidref); + array = hashmap_get(m->watch_pids_more, &pidref); if (array) { size_t n = 0; diff --git a/src/core/manager.h b/src/core/manager.h index b06270fccc6..05697ce4e84 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -242,14 +242,15 @@ struct Manager { sd_event *event; - /* This maps PIDs we care about to units that are interested in. We allow multiple units to be interested in - * the same PID and multiple PIDs to be relevant to the same unit. Since in most cases only a single unit will - * be interested in the same PID we use a somewhat special encoding here: the first unit interested in a PID is - * stored directly in the hashmap, keyed by the PID unmodified. If there are other units interested too they'll - * be stored in a NULL-terminated array, and keyed by the negative PID. This is safe as pid_t is signed and - * negative PIDs are not used for regular processes but process groups, which we don't care about in this - * context, but this allows us to use the negative range for our own purposes. */ - Hashmap *watch_pids; /* pid => unit as well as -pid => array of units */ + /* This maps PIDs we care about to units that are interested in them. We allow multiple units to be + * interested in the same PID and multiple PIDs to be relevant to the same unit. Since in most cases + * only a single unit will be interested in the same PID though, we use a somewhat special structure + * here: the first unit interested in a PID is stored in the hashmap 'watch_pids', keyed by the + * PID. If there are other units interested too they'll be stored in a NULL-terminated array, stored + * in the hashmap 'watch_pids_more', keyed by the PID. Thus to go through the full list of units + * interested in a PID we must look into both hashmaps. */ + Hashmap *watch_pids; /* PidRef* → Unit* */ + Hashmap *watch_pids_more; /* PidRef* → NUL terminated array of Unit* */ /* A set contains all units which cgroup should be refreshed after startup */ Set *startup_units; @@ -544,7 +545,7 @@ int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, sd_bus_error void manager_clear_jobs(Manager *m); -void manager_unwatch_pid(Manager *m, pid_t pid); +void manager_unwatch_pidref(Manager *m, PidRef *pid); unsigned manager_dispatch_load_queue(Manager *m); diff --git a/src/core/mount.c b/src/core/mount.c index e7a18d13b79..17925f50d90 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -203,7 +203,7 @@ static void mount_unwatch_control_pid(Mount *m) { if (!pidref_is_set(&m->control_pid)) return; - unit_unwatch_pid(UNIT(m), m->control_pid.pid); + unit_unwatch_pidref(UNIT(m), &m->control_pid); pidref_done(&m->control_pid); } @@ -781,7 +781,7 @@ static int mount_coldplug(Unit *u) { pid_is_unwaited(m->control_pid.pid) && MOUNT_STATE_WITH_PROCESS(m->deserialized_state)) { - r = unit_watch_pid(UNIT(m), m->control_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(m), &m->control_pid, /* exclusive= */ false); if (r < 0) return r; @@ -930,7 +930,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, PidRef *ret_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(m), pidref.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(m), &pidref, /* exclusive= */ true); if (r < 0) return r; diff --git a/src/core/scope.c b/src/core/scope.c index cd6fafe0de2..69ac20e53c1 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -242,10 +242,10 @@ static int scope_coldplug(Unit *u) { if (!IN_SET(s->deserialized_state, SCOPE_DEAD, SCOPE_FAILED)) { if (u->pids) { - void *pidp; + PidRef *pid; - SET_FOREACH(pidp, u->pids) { - r = unit_watch_pid(u, PTR_TO_PID(pidp), false); + SET_FOREACH(pid, u->pids) { + r = unit_watch_pidref(u, pid, /* exclusive= */ false); if (r < 0 && r != -EEXIST) return r; } @@ -398,7 +398,7 @@ static int scope_enter_start_chown(Scope *s) { _exit(EXIT_SUCCESS); } - r = unit_watch_pid(UNIT(s), pidref.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(s), &pidref, /* exclusive= */ true); if (r < 0) goto fail; @@ -533,7 +533,7 @@ static int scope_get_timeout(Unit *u, usec_t *timeout) { static int scope_serialize(Unit *u, FILE *f, FDSet *fds) { Scope *s = SCOPE(u); - void *pidp; + PidRef *pid; assert(s); assert(f); @@ -545,8 +545,8 @@ static int scope_serialize(Unit *u, FILE *f, FDSet *fds) { if (s->controller) (void) serialize_item(f, "controller", s->controller); - SET_FOREACH(pidp, u->pids) - serialize_item_format(f, "pids", PID_FMT, PTR_TO_PID(pidp)); + SET_FOREACH(pid, u->pids) + serialize_item_format(f, "pids", PID_FMT, pid->pid); return 0; } @@ -584,15 +584,10 @@ static int scope_deserialize_item(Unit *u, const char *key, const char *value, F return log_oom(); } else if (streq(key, "pids")) { - pid_t pid; - - if (parse_pid(value, &pid) < 0) - log_unit_debug(u, "Failed to parse pids value: %s", value); - else { - r = set_ensure_put(&u->pids, NULL, PID_TO_PTR(pid)); - if (r < 0) - return r; - } + + r = unit_watch_pid_str(u, value, /* exclusive= */ false); + if (r < 0) + log_unit_debug(u, "Failed to parse pids value, ignoring: %s", value); } else log_unit_debug(u, "Unknown serialization key: %s", key); diff --git a/src/core/service.c b/src/core/service.c index fc2380c5a95..8dc98813502 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -155,7 +155,7 @@ static void service_unwatch_control_pid(Service *s) { if (!pidref_is_set(&s->control_pid)) return; - unit_unwatch_pid(UNIT(s), s->control_pid.pid); + unit_unwatch_pidref(UNIT(s), &s->control_pid); pidref_done(&s->control_pid); } @@ -165,7 +165,7 @@ static void service_unwatch_main_pid(Service *s) { if (!pidref_is_set(&s->main_pid)) return; - unit_unwatch_pid(UNIT(s), s->main_pid.pid); + unit_unwatch_pidref(UNIT(s), &s->main_pid); pidref_done(&s->main_pid); } @@ -1064,28 +1064,28 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { cgroup_context_dump(UNIT(s), f, prefix); } -static int service_is_suitable_main_pid(Service *s, pid_t pid, int prio) { +static int service_is_suitable_main_pid(Service *s, PidRef *pid, int prio) { Unit *owner; assert(s); - assert(pid_is_valid(pid)); + assert(pidref_is_set(pid)); /* Checks whether the specified PID is suitable as main PID for this service. returns negative if not, 0 if the * PID is questionnable but should be accepted if the source of configuration is trusted. > 0 if the PID is * good */ - if (pid == getpid_cached() || pid == 1) - return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(EPERM), "New main PID "PID_FMT" is the manager, refusing.", pid); + if (pid->pid == getpid_cached() || pid->pid == 1) + return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(EPERM), "New main PID "PID_FMT" is the manager, refusing.", pid->pid); - if (pid == s->control_pid.pid) - return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(EPERM), "New main PID "PID_FMT" is the control process, refusing.", pid); + if (pidref_equal(pid, &s->control_pid)) + return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(EPERM), "New main PID "PID_FMT" is the control process, refusing.", pid->pid); - if (!pid_is_alive(pid)) - return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(ESRCH), "New main PID "PID_FMT" does not exist or is a zombie.", pid); + if (!pid_is_alive(pid->pid)) + return log_unit_full_errno(UNIT(s), prio, SYNTHETIC_ERRNO(ESRCH), "New main PID "PID_FMT" does not exist or is a zombie.", pid->pid); - owner = manager_get_unit_by_pid(UNIT(s)->manager, pid); + owner = manager_get_unit_by_pidref(UNIT(s)->manager, pid); if (owner == UNIT(s)) { - log_unit_debug(UNIT(s), "New main PID "PID_FMT" belongs to service, we are happy.", pid); + log_unit_debug(UNIT(s), "New main PID "PID_FMT" belongs to service, we are happy.", pid->pid); return 1; /* Yay, it's definitely a good PID */ } @@ -1098,7 +1098,6 @@ static int service_load_pid_file(Service *s, bool may_warn) { _cleanup_free_ char *k = NULL; _cleanup_close_ int fd = -EBADF; int r, prio; - pid_t pid; assert(s); @@ -1128,18 +1127,14 @@ static int service_load_pid_file(Service *s, bool may_warn) { "Can't convert PID files %s O_PATH file descriptor to proper file descriptor: %m", s->pid_file); - r = parse_pid(k, &pid); + r = pidref_set_pidstr(&pidref, k); if (r < 0) return log_unit_full_errno(UNIT(s), prio, r, "Failed to parse PID from file %s: %m", s->pid_file); - if (s->main_pid_known && pid == s->main_pid.pid) + if (s->main_pid_known && pidref_equal(&pidref, &s->main_pid)) return 0; - r = pidref_set_pid(&pidref, pid); - if (r < 0) - return log_unit_full_errno(UNIT(s), prio, r, "Failed to pin PID " PID_FMT ": %m", pid); - - r = service_is_suitable_main_pid(s, pidref.pid, prio); + r = service_is_suitable_main_pid(s, &pidref, prio); if (r < 0) return r; if (r == 0) { @@ -1173,7 +1168,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), s->main_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->main_pid, /* exclusive= */ false); if (r < 0) /* FIXME: we need to do something here */ return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", s->main_pid.pid); @@ -1181,7 +1176,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { } static void service_search_main_pid(Service *s) { - pid_t pid = 0; + _cleanup_(pidref_done) PidRef pid = PIDREF_NULL; int r; assert(s); @@ -1198,11 +1193,11 @@ static void service_search_main_pid(Service *s) { if (unit_search_main_pid(UNIT(s), &pid) < 0) return; - log_unit_debug(UNIT(s), "Main PID guessed: "PID_FMT, pid); - if (service_set_main_pid(s, pid) < 0) + log_unit_debug(UNIT(s), "Main PID guessed: "PID_FMT, pid.pid); + if (service_set_main_pidref(s, &pid) < 0) return; - r = unit_watch_pid(UNIT(s), s->main_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->main_pid, /* exclusive= */ false); if (r < 0) /* FIXME: we need to do something here */ log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" from: %m", s->main_pid.pid); @@ -1342,7 +1337,7 @@ static int service_coldplug(Unit *u) { SERVICE_RELOAD, SERVICE_RELOAD_SIGNAL, SERVICE_RELOAD_NOTIFY, SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_WATCHDOG, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL))) { - r = unit_watch_pid(UNIT(s), s->main_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->main_pid, /* exclusive= */ false); if (r < 0) return r; } @@ -1355,7 +1350,7 @@ static int service_coldplug(Unit *u) { SERVICE_STOP, SERVICE_STOP_WATCHDOG, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST, SERVICE_FINAL_WATCHDOG, SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL, SERVICE_CLEANING)) { - r = unit_watch_pid(UNIT(s), s->control_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->control_pid, /* exclusive= */ false); if (r < 0) return r; } @@ -1833,7 +1828,7 @@ static int service_spawn_internal( if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pidref.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(s), &pidref, /* exclusive= */ true); if (r < 0) return r; @@ -4399,28 +4394,29 @@ static void service_notify_message( /* Interpret MAINPID= */ e = strv_find_startswith(tags, "MAINPID="); if (e && IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD, SERVICE_RELOAD_SIGNAL, SERVICE_RELOAD_NOTIFY)) { - pid_t new_main_pid; + _cleanup_(pidref_done) PidRef new_main_pid = PIDREF_NULL; - if (parse_pid(e, &new_main_pid) < 0) - log_unit_warning(u, "Failed to parse MAINPID= field in notification message, ignoring: %s", e); - else if (!s->main_pid_known || new_main_pid != s->main_pid.pid) { + r = pidref_set_pidstr(&new_main_pid, e); + if (r < 0) + log_unit_warning_errno(u, r, "Failed to parse MAINPID=%s field in notification message, ignoring: %m", e); + else if (!s->main_pid_known || !pidref_equal(&new_main_pid, &s->main_pid)) { - r = service_is_suitable_main_pid(s, new_main_pid, LOG_WARNING); + r = service_is_suitable_main_pid(s, &new_main_pid, LOG_WARNING); if (r == 0) { /* The new main PID is a bit suspicious, which is OK if the sender is privileged. */ if (ucred->uid == 0) { - log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, but we'll accept it as the request to change it came from a privileged process.", new_main_pid); + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, but we'll accept it as the request to change it came from a privileged process.", new_main_pid.pid); r = 1; } else - log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, refusing.", new_main_pid); + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, refusing.", new_main_pid.pid); } if (r > 0) { - (void) service_set_main_pid(s, new_main_pid); + (void) service_set_main_pidref(s, &new_main_pid); - r = unit_watch_pid(UNIT(s), new_main_pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->main_pid, /* exclusive= */ false); if (r < 0) - log_unit_warning_errno(UNIT(s), r, "Failed to watch new main PID "PID_FMT" for service: %m", new_main_pid); + log_unit_warning_errno(UNIT(s), r, "Failed to watch new main PID "PID_FMT" for service: %m", s->main_pid.pid); notify_dbus = true; } @@ -4653,6 +4649,7 @@ static bool pick_up_pid_from_bus_name(Service *s) { } static int bus_name_pid_lookup_callback(sd_bus_message *reply, void *userdata, sd_bus_error *ret_error) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; const sd_bus_error *e; Unit *u = ASSERT_PTR(userdata); uint32_t pid; @@ -4680,15 +4677,16 @@ static int bus_name_pid_lookup_callback(sd_bus_message *reply, void *userdata, s return 1; } - if (!pid_is_valid(pid)) { - log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "GetConnectionUnixProcessID() returned invalid PID"); + r = pidref_set_pid(&pidref, pid); + if (r < 0) { + log_debug_errno(r, "GetConnectionUnixProcessID() returned invalid PID: %m"); return 1; } - log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, s->bus_name, (pid_t) pid); + log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, s->bus_name, pidref.pid); - (void) service_set_main_pid(s, pid); - (void) unit_watch_pid(UNIT(s), pid, /* exclusive= */ false); + (void) service_set_main_pidref(s, &pidref); + (void) unit_watch_pidref(UNIT(s), &s->main_pid, /* exclusive= */ false); return 1; } diff --git a/src/core/socket.c b/src/core/socket.c index 91951460656..e5b2e8ae762 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -113,7 +113,7 @@ static void socket_unwatch_control_pid(Socket *s) { if (!pidref_is_set(&s->control_pid)) return; - unit_unwatch_pid(UNIT(s), s->control_pid.pid); + unit_unwatch_pidref(UNIT(s), &s->control_pid); pidref_done(&s->control_pid); } @@ -1865,7 +1865,7 @@ static int socket_coldplug(Unit *u) { SOCKET_FINAL_SIGKILL, SOCKET_CLEANING)) { - r = unit_watch_pid(UNIT(s), s->control_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->control_pid, /* exclusive= */ false); if (r < 0) return r; @@ -1954,7 +1954,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, PidRef *ret_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pidref.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(s), &pidref, /* exclusive= */ true); if (r < 0) return r; @@ -2024,7 +2024,7 @@ static int socket_chown(Socket *s, PidRef *ret_pid) { _exit(EXIT_SUCCESS); } - r = unit_watch_pid(UNIT(s), pid.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(s), &pid, /* exclusive= */ true); if (r < 0) return r; diff --git a/src/core/swap.c b/src/core/swap.c index e849e33a421..8a6a32543a9 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -156,7 +156,7 @@ static void swap_unwatch_control_pid(Swap *s) { if (!pidref_is_set(&s->control_pid)) return; - unit_unwatch_pid(UNIT(s), s->control_pid.pid); + unit_unwatch_pidref(UNIT(s), &s->control_pid); pidref_done(&s->control_pid); } @@ -560,7 +560,7 @@ static int swap_coldplug(Unit *u) { pid_is_unwaited(s->control_pid.pid) && SWAP_STATE_WITH_PROCESS(new_state)) { - r = unit_watch_pid(UNIT(s), s->control_pid.pid, /* exclusive= */ false); + r = unit_watch_pidref(UNIT(s), &s->control_pid, /* exclusive= */ false); if (r < 0) return r; @@ -673,7 +673,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, PidRef *ret_pid) { if (r < 0) return r; - r = unit_watch_pid(UNIT(s), pidref.pid, /* exclusive= */ true); + r = unit_watch_pidref(UNIT(s), &pidref, /* exclusive= */ true); if (r < 0) return r; diff --git a/src/core/unit.c b/src/core/unit.c index 776005489c9..ea49cfa504f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2834,117 +2834,168 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su } } -int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { +int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive) { + _cleanup_(pidref_freep) PidRef *pid_dup = NULL; int r; - assert(u); - assert(pid_is_valid(pid)); + /* Adds a specific PID to the set of PIDs this unit watches. */ - /* Watch a specific PID */ + assert(u); + assert(pidref_is_set(pid)); /* Caller might be sure that this PID belongs to this unit only. Let's take this * opportunity to remove any stalled references to this PID as they can be created * easily (when watching a process which is not our direct child). */ if (exclusive) - manager_unwatch_pid(u->manager, pid); + manager_unwatch_pidref(u->manager, pid); + + if (set_contains(u->pids, pid)) /* early exit if already being watched */ + return 0; - r = set_ensure_allocated(&u->pids, NULL); + r = pidref_dup(pid, &pid_dup); if (r < 0) return r; - r = hashmap_ensure_allocated(&u->manager->watch_pids, NULL); + /* First, insert into the set of PIDs maintained by the unit */ + r = set_ensure_put(&u->pids, &pidref_hash_ops, pid_dup); if (r < 0) return r; - /* First try, let's add the unit keyed by "pid". */ - r = hashmap_put(u->manager->watch_pids, PID_TO_PTR(pid), u); - if (r == -EEXIST) { - Unit **array; - bool found = false; - size_t n = 0; + pid = TAKE_PTR(pid_dup); /* continue with our copy now that we have installed it properly in our set */ - /* OK, the "pid" key is already assigned to a different unit. Let's see if the "-pid" key (which points - * to an array of Units rather than just a Unit), lists us already. */ + /* Second, insert it into the simple global table, see if that works */ + r = hashmap_ensure_put(&u->manager->watch_pids, &pidref_hash_ops, pid, u); + if (r != -EEXIST) + return r; - array = hashmap_get(u->manager->watch_pids, PID_TO_PTR(-pid)); - if (array) - for (; array[n]; n++) - if (array[n] == u) - found = true; + /* OK, the key is already assigned to a different unit. That's fine, then add us via the second + * hashmap that points to an array. */ - if (!found) { - Unit **new_array; + PidRef *old_pid = NULL; + Unit **array = hashmap_get2(u->manager->watch_pids_more, pid, (void**) &old_pid); - /* Allocate a new array */ - new_array = new(Unit*, n + 2); - if (!new_array) - return -ENOMEM; + /* Count entries in array */ + size_t n = 0; + for (; array && array[n]; n++) + ; - memcpy_safe(new_array, array, sizeof(Unit*) * n); - new_array[n] = u; - new_array[n+1] = NULL; + /* Allocate a new array */ + _cleanup_free_ Unit **new_array = new(Unit*, n + 2); + if (!new_array) + return -ENOMEM; - /* Add or replace the old array */ - r = hashmap_replace(u->manager->watch_pids, PID_TO_PTR(-pid), new_array); - if (r < 0) { - free(new_array); - return r; - } + /* Append us to the end */ + memcpy_safe(new_array, array, sizeof(Unit*) * n); + new_array[n] = u; + new_array[n+1] = NULL; - free(array); - } - } else if (r < 0) + /* Make sure the hashmap is allocated */ + r = hashmap_ensure_allocated(&u->manager->watch_pids_more, &pidref_hash_ops); + if (r < 0) return r; - r = set_put(u->pids, PID_TO_PTR(pid)); + /* Add or replace the old array */ + r = hashmap_replace(u->manager->watch_pids_more, old_pid ?: pid, new_array); if (r < 0) return r; + TAKE_PTR(new_array); /* Now part of the hash table */ + free(array); /* Which means we can now delete the old version */ return 0; } -void unit_unwatch_pid(Unit *u, pid_t pid) { - Unit **array; +int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + int r; assert(u); assert(pid_is_valid(pid)); - /* First let's drop the unit in case it's keyed as "pid". */ - (void) hashmap_remove_value(u->manager->watch_pids, PID_TO_PTR(pid), u); + r = pidref_set_pid(&pidref, pid); + if (r < 0) + return r; - /* Then, let's also drop the unit, in case it's in the array keyed by -pid */ - array = hashmap_get(u->manager->watch_pids, PID_TO_PTR(-pid)); - if (array) { - /* Let's iterate through the array, dropping our own entry */ + return unit_watch_pidref(u, &pidref, exclusive); +} - size_t m = 0; - for (size_t n = 0; array[n]; n++) +int unit_watch_pid_str(Unit *u, const char *s, bool exclusive) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + int r; + + assert(u); + assert(s); + + r = pidref_set_pidstr(&pidref, s); + if (r < 0) + return r; + + return unit_watch_pidref(u, &pidref, exclusive); +} + +void unit_unwatch_pidref(Unit *u, PidRef *pid) { + assert(u); + assert(pidref_is_set(pid)); + + /* Remove from the set we maintain for this unit. (And destroy the returned pid eventually) */ + _cleanup_(pidref_freep) PidRef *pid1 = set_remove(u->pids, pid); + if (!pid1) + return; /* Early exit if this PID was never watched by us */ + + /* First let's drop the unit from the simple hash table, if it is included there */ + PidRef *pid2 = NULL; + Unit *uu = hashmap_get2(u->manager->watch_pids, pid, (void**) &pid2); + + /* Quick validation: iff we are in the watch_pids table then the PidRef object must be the same as in our local pids set */ + assert((uu == u) == (pid1 == pid2)); + + if (uu == u) + /* OK, we are in the first table. Let's remove it there then, and we are done already. */ + assert_se(hashmap_remove_value(u->manager->watch_pids, pid2, uu) == uu); + else { + /* We weren't in the first table, then let's consult the 2nd table that points to an array */ + PidRef *pid3 = NULL; + Unit **array = hashmap_get2(u->manager->watch_pids_more, pid, (void**) &pid3); + + /* Let's iterate through the array, dropping our own entry */ + size_t m = 0, n = 0; + for (; array && array[n]; n++) if (array[n] != u) array[m++] = array[n]; - array[m] = NULL; + if (n == m) + return; /* Not there */ + + array[m] = NULL; /* set trailing NULL marker on the new end */ if (m == 0) { /* The array is now empty, remove the entire entry */ - assert_se(hashmap_remove(u->manager->watch_pids, PID_TO_PTR(-pid)) == array); + assert_se(hashmap_remove_value(u->manager->watch_pids_more, pid3, array) == array); free(array); + } else { + /* The array is not empty, but let's make sure the entry is not keyed by the PidRef + * we will delete, but by the PidRef object of the Unit that is now first in the + * array. */ + + PidRef *new_pid3 = ASSERT_PTR(set_get(array[0]->pids, pid)); + assert_se(hashmap_replace(u->manager->watch_pids_more, new_pid3, array) >= 0); } } +} - (void) set_remove(u->pids, PID_TO_PTR(pid)); +void unit_unwatch_pid(Unit *u, pid_t pid) { + return unit_unwatch_pidref(u, &PIDREF_MAKE_FROM_PID(pid)); } void unit_unwatch_all_pids(Unit *u) { assert(u); while (!set_isempty(u->pids)) - unit_unwatch_pid(u, PTR_TO_PID(set_first(u->pids))); + unit_unwatch_pidref(u, set_first(u->pids)); u->pids = set_free(u->pids); } static void unit_tidy_watch_pids(Unit *u) { - PidRef *except1, *except2; - void *e; + PidRef *except1, *except2, *e; assert(u); @@ -2954,14 +3005,11 @@ static void unit_tidy_watch_pids(Unit *u) { except2 = unit_control_pid(u); SET_FOREACH(e, u->pids) { - pid_t pid = PTR_TO_PID(e); - - if ((pidref_is_set(except1) && pid == except1->pid) || - (pidref_is_set(except2) && pid == except2->pid)) + if (pidref_equal(except1, e) || pidref_equal(except2, e)) continue; - if (!pid_is_unwaited(pid)) - unit_unwatch_pid(u, pid); + if (!pid_is_unwaited(e->pid)) + unit_unwatch_pidref(u, e); } } @@ -5431,7 +5479,7 @@ int unit_fork_and_watch_rm_rf(Unit *u, char **paths, PidRef *ret_pid) { _exit(ret); } - r = unit_watch_pid(u, pid.pid, /* exclusive= */ true); + r = unit_watch_pidref(u, &pid, /* exclusive= */ true); if (r < 0) return r; @@ -5880,7 +5928,7 @@ bool unit_needs_console(Unit *u) { return exec_context_may_touch_console(ec); } -int unit_pid_attachable(Unit *u, pid_t pid, sd_bus_error *error) { +int unit_pid_attachable(Unit *u, PidRef *pid, sd_bus_error *error) { int r; assert(u); @@ -5889,21 +5937,21 @@ int unit_pid_attachable(Unit *u, pid_t pid, sd_bus_error *error) { * and not a kernel thread either */ /* First, a simple range check */ - if (!pid_is_valid(pid)) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process identifier " PID_FMT " is not valid.", pid); + if (!pidref_is_set(pid)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process identifier is not valid."); /* Some extra safety check */ - if (pid == 1 || pid == getpid_cached()) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process " PID_FMT " is a manager process, refusing.", pid); + if (pid->pid == 1 || pid->pid == getpid_cached()) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process " PID_FMT " is a manager process, refusing.", pid->pid); /* Don't even begin to bother with kernel threads */ - r = is_kernel_thread(pid); + r = is_kernel_thread(pid->pid); if (r == -ESRCH) - return sd_bus_error_setf(error, SD_BUS_ERROR_UNIX_PROCESS_ID_UNKNOWN, "Process with ID " PID_FMT " does not exist.", pid); + return sd_bus_error_setf(error, SD_BUS_ERROR_UNIX_PROCESS_ID_UNKNOWN, "Process with ID " PID_FMT " does not exist.", pid->pid); if (r < 0) - return sd_bus_error_set_errnof(error, r, "Failed to determine whether process " PID_FMT " is a kernel thread: %m", pid); + return sd_bus_error_set_errnof(error, r, "Failed to determine whether process " PID_FMT " is a kernel thread: %m", pid->pid); if (r > 0) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process " PID_FMT " is a kernel thread, refusing.", pid); + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Process " PID_FMT " is a kernel thread, refusing.", pid->pid); return 0; } diff --git a/src/core/unit.h b/src/core/unit.h index 2dc9c41cef6..b39d868f869 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -322,10 +322,9 @@ typedef struct Unit { /* Queue of units that should be checked if they can release resources now */ LIST_FIELDS(Unit, release_resources_queue); - /* PIDs we keep an eye on. Note that a unit might have many - * more, but these are the ones we care enough about to - * process SIGCHLD for */ - Set *pids; + /* PIDs we keep an eye on. Note that a unit might have many more, but these are the ones we care + * enough about to process SIGCHLD for */ + Set *pids; /* → PidRef* */ /* Used in SIGCHLD and sd_notify() message event invocation logic to avoid that we dispatch the same event * multiple times on the same unit. */ @@ -920,7 +919,10 @@ void unit_notify_cgroup_oom(Unit *u, bool managed_oom); void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success); +int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive); int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); +int unit_watch_pid_str(Unit *u, const char *s, bool exclusive); +void unit_unwatch_pidref(Unit *u, PidRef *pid); void unit_unwatch_pid(Unit *u, pid_t pid); void unit_unwatch_all_pids(Unit *u); @@ -1043,7 +1045,7 @@ int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func); bool unit_needs_console(Unit *u); -int unit_pid_attachable(Unit *unit, pid_t pid, sd_bus_error *error); +int unit_pid_attachable(Unit *unit, PidRef *pid, sd_bus_error *error); static inline bool unit_has_job_type(Unit *u, JobType type) { return u && u->job && u->job->type == type; -- 2.39.5