Merge pull request #29976 from YHNdnzj/session-by-leader-pidref

logind: switch sessions_by_leader to PidRef

src/basic/cgroup-util.c

@@ -1262,6 +1262,27 @@ int cg_pid_get_unit(pid_t pid, char **ret_unit) {
         return cg_path_get_unit(cgroup, ret_unit);
 }
 
+int cg_pidref_get_unit(const PidRef *pidref, char **ret) {
+        _cleanup_free_ char *unit = NULL;
+        int r;
+
+        assert(ret);
+
+        if (!pidref_is_set(pidref))
+                return -ESRCH;
+
+        r = cg_pid_get_unit(pidref->pid, &unit);
+        if (r < 0)
+                return r;
+
+        r = pidref_verify(pidref);
+        if (r < 0)
+                return r;
+
+        *ret = TAKE_PTR(unit);
+        return 0;
+}
+
 /**
  * Skip session-*.scope, but require it to be there.
  */

src/basic/cgroup-util.h

@@ -279,6 +279,7 @@ int cg_pid_get_path_shifted(pid_t pid, const char *cached_root, char **ret_cgrou
 int cg_pid_get_session(pid_t pid, char **ret_session);
 int cg_pid_get_owner_uid(pid_t pid, uid_t *ret_uid);
 int cg_pid_get_unit(pid_t pid, char **ret_unit);
+int cg_pidref_get_unit(const PidRef *pidref, char **ret);
 int cg_pid_get_user_unit(pid_t pid, char **ret_unit);
 int cg_pid_get_machine_name(pid_t pid, char **ret_machine);
 int cg_pid_get_slice(pid_t pid, char **ret_slice);

src/basic/pidref.c

@@ -278,9 +278,8 @@ static int pidref_compare_func(const PidRef *a, const PidRef *b) {
         return CMP(a->pid, b->pid);
 }
 
-DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(
-                pidref_hash_ops,
-                PidRef,
-                pidref_hash_func,
-                pidref_compare_func,
-                pidref_free);
+DEFINE_HASH_OPS(pidref_hash_ops, PidRef, pidref_hash_func, pidref_compare_func);
+
+DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(pidref_hash_ops_free,
+                                    PidRef, pidref_hash_func, pidref_compare_func,
+                                    pidref_free);

src/basic/pidref.h

@@ -61,4 +61,5 @@ int pidref_verify(const PidRef *pidref);
 
 #define TAKE_PIDREF(p) TAKE_GENERIC((p), PidRef, PIDREF_NULL)
 
-extern const struct hash_ops pidref_hash_ops; /* Has destructor call for pidref_free(), i.e. expects heap allocated PidRef as keys */
+extern const struct hash_ops pidref_hash_ops;
+extern const struct hash_ops pidref_hash_ops_free; /* Has destructor call for pidref_free(), i.e. expects heap allocated PidRef as keys */

src/core/dbus-unit.c

@@ -1583,7 +1583,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);
                 }
 
-                r = set_ensure_consume(&pids, &pidref_hash_ops, TAKE_PTR(pidref));
+                r = set_ensure_consume(&pids, &pidref_hash_ops_free, TAKE_PTR(pidref));
                 if (r < 0)
                         return r;
         }

src/core/unit.c

@@ -2884,14 +2884,14 @@ int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive) {
                 return r;
 
         /* First, insert into the set of PIDs maintained by the unit */
-        r = set_ensure_put(&u->pids, &pidref_hash_ops, pid_dup);
+        r = set_ensure_put(&u->pids, &pidref_hash_ops_free, pid_dup);
         if (r < 0)
                 return r;
 
         pid = TAKE_PTR(pid_dup); /* continue with our copy now that we have installed it properly in our set */
 
         /* 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);
+        r = hashmap_ensure_put(&u->manager->watch_pids, &pidref_hash_ops_free, pid, u);
         if (r != -EEXIST)
                 return r;
 
@@ -2917,7 +2917,7 @@ int unit_watch_pidref(Unit *u, PidRef *pid, bool exclusive) {
         new_array[n+1] = NULL;
 
         /* Make sure the hashmap is allocated */
-        r = hashmap_ensure_allocated(&u->manager->watch_pids_more, &pidref_hash_ops);
+        r = hashmap_ensure_allocated(&u->manager->watch_pids_more, &pidref_hash_ops_free);
         if (r < 0)
                 return r;
 
@@ -2963,7 +2963,7 @@ void unit_unwatch_pidref(Unit *u, PidRef *pid) {
 
         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);
+                assert_se(hashmap_remove_value(u->manager->watch_pids, pid2, uu));
         else {
                 /* We weren't in the first table, then let's consult the 2nd table that points to an array */
                 PidRef *pid3 = NULL;
@@ -2981,7 +2981,7 @@ void unit_unwatch_pidref(Unit *u, PidRef *pid) {
 
                 if (m == 0) {
                         /* The array is now empty, remove the entire entry */
-                        assert_se(hashmap_remove_value(u->manager->watch_pids_more, pid3, array) == array);
+                        assert_se(hashmap_remove_value(u->manager->watch_pids_more, pid3, array));
                         free(array);
                 } else {
                         /* The array is not empty, but let's make sure the entry is not keyed by the PidRef

src/login/logind-core.c

@@ -349,21 +349,27 @@ int manager_process_button_device(Manager *m, sd_device *d) {
         return 0;
 }
 
-int manager_get_session_by_pid(Manager *m, pid_t pid, Session **ret) {
+int manager_get_session_by_pidref(Manager *m, const PidRef *pid, Session **ret) {
         _cleanup_free_ char *unit = NULL;
         Session *s;
         int r;
 
         assert(m);
 
-        if (!pid_is_valid(pid))
+        if (!pidref_is_set(pid))
                 return -EINVAL;
 
-        s = hashmap_get(m->sessions_by_leader, PID_TO_PTR(pid));
-        if (!s) {
-                r = cg_pid_get_unit(pid, &unit);
-                if (r >= 0)
-                        s = hashmap_get(m->session_units, unit);
+        s = hashmap_get(m->sessions_by_leader, pid);
+        if (s) {
+                r = pidref_verify(pid);
+                if (r < 0)
+                        return r;
+        } else {
+                r = cg_pidref_get_unit(pid, &unit);
+                if (r < 0)
+                        return r;
+
+                s = hashmap_get(m->session_units, unit);
         }
 
         if (ret)
@@ -734,8 +740,7 @@ int manager_read_utmp(Manager *m) {
                 if (isempty(t))
                         continue;
 
-                s = hashmap_get(m->sessions_by_leader, PID_TO_PTR(u->ut_pid));
-                if (!s)
+                if (manager_get_session_by_pidref(m, &PIDREF_MAKE_FROM_PID(u->ut_pid), &s) <= 0)
                         continue;
 
                 if (s->tty_validity == TTY_FROM_UTMP && !streq_ptr(s->tty, t)) {

src/login/logind-dbus.c

@@ -405,9 +405,9 @@ static int method_get_session(sd_bus_message *message, void *userdata, sd_bus_er
  * as apps may instead belong to a user service unit.  This includes terminal
  * emulators and hence command-line apps. */
 static int method_get_session_by_pid(sd_bus_message *message, void *userdata, sd_bus_error *error) {
+        Manager *m = ASSERT_PTR(userdata);
         _cleanup_free_ char *p = NULL;
         Session *session = NULL;
-        Manager *m = ASSERT_PTR(userdata);
         pid_t pid;
         int r;
 
@@ -426,7 +426,7 @@ static int method_get_session_by_pid(sd_bus_message *message, void *userdata, sd
                 if (r < 0)
                         return r;
         } else {
-                r = manager_get_session_by_pid(m, pid, &session);
+                r = manager_get_session_by_pidref(m, &PIDREF_MAKE_FROM_PID(pid), &session);
                 if (r < 0)
                         return r;
 

src/login/logind-inhibit.c

@@ -376,18 +376,20 @@ InhibitWhat manager_inhibit_what(Manager *m, InhibitMode mm) {
         return what;
 }
 
-static int pid_is_active(Manager *m, pid_t pid) {
+static int pidref_is_active_session(Manager *m, const PidRef *pid) {
         Session *s;
         int r;
 
-        /* Get client session.  This is not what you are looking for these days.
+        assert(m);
+        assert(pid);
+
+        /* Get client session. This is not what you are looking for these days.
          * FIXME #6852 */
-        r = manager_get_session_by_pid(m, pid, &s);
+        r = manager_get_session_by_pidref(m, pid, &s);
         if (r < 0)
                 return r;
 
-        /* If there's no session assigned to it, then it's globally
-         * active on all ttys */
+        /* If there's no session assigned to it, then it's globally active on all ttys */
         if (r == 0)
                 return 1;
 
@@ -421,7 +423,7 @@ bool manager_is_inhibited(
                 if (i->mode != mm)
                         continue;
 
-                if (ignore_inactive && pid_is_active(m, i->pid.pid) <= 0)
+                if (ignore_inactive && pidref_is_active_session(m, &i->pid) <= 0)
                         continue;
 
                 if (ignore_uid && i->uid == uid)

src/login/logind-session.c

@@ -87,6 +87,17 @@ int session_new(Session **ret, Manager *m, const char *id) {
         return 0;
 }
 
+static void session_reset_leader(Session *s) {
+        assert(s);
+
+        if (!pidref_is_set(&s->leader))
+                return;
+
+        assert_se(hashmap_remove_value(s->manager->sessions_by_leader, &s->leader, s));
+
+        return pidref_done(&s->leader);
+}
+
 Session* session_free(Session *s) {
         SessionDevice *sd;
 
@@ -129,13 +140,10 @@ Session* session_free(Session *s) {
                 free(s->scope);
         }
 
-        if (pidref_is_set(&s->leader)) {
-                (void) hashmap_remove_value(s->manager->sessions_by_leader, PID_TO_PTR(s->leader.pid), s);
-                pidref_done(&s->leader);
-        }
-
         free(s->scope_job);
 
+        session_reset_leader(s);
+
         sd_bus_message_unref(s->create_message);
 
         free(s->tty);
@@ -180,16 +188,15 @@ int session_set_leader_consume(Session *s, PidRef _leader) {
         if (pidref_equal(&s->leader, &pidref))
                 return 0;
 
-        r = hashmap_put(s->manager->sessions_by_leader, PID_TO_PTR(pidref.pid), s);
-        if (r < 0)
-                return r;
-
-        if (pidref_is_set(&s->leader)) {
-                (void) hashmap_remove_value(s->manager->sessions_by_leader, PID_TO_PTR(s->leader.pid), s);
-                pidref_done(&s->leader);
-        }
+        session_reset_leader(s);
 
         s->leader = TAKE_PIDREF(pidref);
+
+        r = hashmap_ensure_put(&s->manager->sessions_by_leader, &pidref_hash_ops, &s->leader, s);
+        if (r < 0)
+                return r;
+        assert(r > 0);
+
         (void) audit_session_from_pid(s->leader.pid, &s->audit_id);
 
         return 1;
@@ -941,7 +948,7 @@ int session_finalize(Session *s) {
                 seat_save(s->seat);
         }
 
-        pidref_done(&s->leader);
+        session_reset_leader(s);
 
         user_save(s->user);
         user_send_changed(s->user, "Display", NULL);

src/login/logind.c

@@ -70,7 +70,6 @@ static int manager_new(Manager **ret) {
         m->devices = hashmap_new(&device_hash_ops);
         m->seats = hashmap_new(&seat_hash_ops);
         m->sessions = hashmap_new(&session_hash_ops);
-        m->sessions_by_leader = hashmap_new(NULL);
         m->users = hashmap_new(&user_hash_ops);
         m->inhibitors = hashmap_new(&inhibitor_hash_ops);
         m->buttons = hashmap_new(&button_hash_ops);
@@ -78,7 +77,7 @@ static int manager_new(Manager **ret) {
         m->user_units = hashmap_new(&string_hash_ops);
         m->session_units = hashmap_new(&string_hash_ops);
 
-        if (!m->devices || !m->seats || !m->sessions || !m->sessions_by_leader || !m->users || !m->inhibitors || !m->buttons || !m->user_units || !m->session_units)
+        if (!m->devices || !m->seats || !m->sessions || !m->users || !m->inhibitors || !m->buttons || !m->user_units || !m->session_units)
                 return -ENOMEM;
 
         r = sd_event_default(&m->event);
@@ -116,7 +115,11 @@ static Manager* manager_free(Manager *m) {
         hashmap_free(m->devices);
         hashmap_free(m->seats);
         hashmap_free(m->sessions);
+
+        /* All records should have been removed by session_free */
+        assert(hashmap_isempty(m->sessions_by_leader));
         hashmap_free(m->sessions_by_leader);
+
         hashmap_free(m->users);
         hashmap_free(m->inhibitors);
         hashmap_free(m->buttons);

src/login/logind.h

@@ -163,7 +163,7 @@ bool manager_shall_kill(Manager *m, const char *user);
 int manager_get_idle_hint(Manager *m, dual_timestamp *t);
 
 int manager_get_user_by_pid(Manager *m, pid_t pid, User **user);
-int manager_get_session_by_pid(Manager *m, pid_t pid, Session **session);
+int manager_get_session_by_pidref(Manager *m, const PidRef *pid, Session **ret);
 
 bool manager_is_lid_closed(Manager *m);
 bool manager_is_docked_or_external_displays(Manager *m);