From 864fe630a7a1f11b735d818b8c79d2d1068e2f3f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:31:06 +0100 Subject: [PATCH 01/10] logind: trivial improvements Just some addition whitespace, some additional assert()s, and removal of redundant variables. --- src/login/logind-core.c | 3 ++ src/login/logind-session-device.c | 51 ++++++++++++++++--------------- src/login/logind.c | 2 +- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/login/logind-core.c b/src/login/logind-core.c index cd10536ce5..41b4d4d8d7 100644 --- a/src/login/logind-core.c +++ b/src/login/logind-core.c @@ -618,6 +618,8 @@ bool manager_is_on_external_power(void) { } bool manager_all_buttons_ignored(Manager *m) { + assert(m); + if (m->handle_power_key != HANDLE_IGNORE) return false; if (m->handle_suspend_key != HANDLE_IGNORE) @@ -631,5 +633,6 @@ bool manager_all_buttons_ignored(Manager *m) { return false; if (m->handle_lid_switch_docked != HANDLE_IGNORE) return false; + return true; } diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 067e67a93d..b1bac04036 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -74,20 +74,25 @@ static int session_device_notify(SessionDevice *sd, enum SessionDeviceNotificati return r; switch (type) { + case SESSION_DEVICE_RESUME: r = sd_bus_message_append(m, "uuh", major, minor, sd->fd); if (r < 0) return r; break; + case SESSION_DEVICE_TRY_PAUSE: t = "pause"; break; + case SESSION_DEVICE_PAUSE: t = "force"; break; + case SESSION_DEVICE_RELEASE: t = "gone"; break; + default: return -EINVAL; } @@ -120,24 +125,18 @@ static int sd_eviocrevoke(int fd) { } static int sd_drmsetmaster(int fd) { - int r; - assert(fd >= 0); - r = ioctl(fd, DRM_IOCTL_SET_MASTER, 0); - if (r < 0) + if (ioctl(fd, DRM_IOCTL_SET_MASTER, 0) < 0) return -errno; return 0; } static int sd_drmdropmaster(int fd) { - int r; - assert(fd >= 0); - r = ioctl(fd, DRM_IOCTL_DROP_MASTER, 0); - if (r < 0) + if (ioctl(fd, DRM_IOCTL_DROP_MASTER, 0) < 0) return -errno; return 0; @@ -146,7 +145,9 @@ static int sd_drmdropmaster(int fd) { static int session_device_open(SessionDevice *sd, bool active) { int fd, r; + assert(sd); assert(sd->type != DEVICE_TYPE_UNKNOWN); + assert(sd->node); /* open device and try to get an udev_device from it */ fd = open(sd->node, O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); @@ -154,28 +155,27 @@ static int session_device_open(SessionDevice *sd, bool active) { return -errno; switch (sd->type) { + case DEVICE_TYPE_DRM: if (active) { - /* Weird legacy DRM semantics might return an error - * even though we're master. No way to detect that so - * fail at all times and let caller retry in inactive - * state. */ + /* Weird legacy DRM semantics might return an error even though we're master. No way to detect + * that so fail at all times and let caller retry in inactive state. */ r = sd_drmsetmaster(fd); if (r < 0) { close_nointr(fd); return r; } - } else { - /* DRM-Master is granted to the first user who opens a - * device automatically (ughh, racy!). Hence, we just - * drop DRM-Master in case we were the first. */ + } else + /* DRM-Master is granted to the first user who opens a device automatically (ughh, + * racy!). Hence, we just drop DRM-Master in case we were the first. */ sd_drmdropmaster(fd); - } break; + case DEVICE_TYPE_EVDEV: if (!active) sd_eviocrevoke(fd); break; + case DEVICE_TYPE_UNKNOWN: default: /* fallback for devices wihout synchronizations */ @@ -195,26 +195,27 @@ static int session_device_start(SessionDevice *sd) { return 0; switch (sd->type) { + case DEVICE_TYPE_DRM: - /* Device is kept open. Simply call drmSetMaster() and hope - * there is no-one else. In case it fails, we keep the device - * paused. Maybe at some point we have a drmStealMaster(). */ + /* Device is kept open. Simply call drmSetMaster() and hope there is no-one else. In case it fails, we + * keep the device paused. Maybe at some point we have a drmStealMaster(). */ r = sd_drmsetmaster(sd->fd); if (r < 0) return r; break; + case DEVICE_TYPE_EVDEV: - /* Evdev devices are revoked while inactive. Reopen it and we - * are fine. */ + /* Evdev devices are revoked while inactive. Reopen it and we are fine. */ r = session_device_open(sd, true); if (r < 0) return r; - /* For evdev devices, the file descriptor might be left - * uninitialized. This might happen while resuming into a - * session and logind has been restarted right before. */ + + /* For evdev devices, the file descriptor might be left uninitialized. This might happen while resuming + * into a session and logind has been restarted right before. */ safe_close(sd->fd); sd->fd = r; break; + case DEVICE_TYPE_UNKNOWN: default: /* fallback for devices wihout synchronizations */ diff --git a/src/login/logind.c b/src/login/logind.c index 434a0698ad..875d467cbb 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -455,7 +455,7 @@ static int manager_attach_fds(Manager *m) { sd = hashmap_get(s->devices, &st.st_rdev); if (!sd) { - /* Weird we got an fd for a session device which wasn't + /* Weird, we got an fd for a session device which wasn't * recorded in the session state file... */ log_warning("Got fd for missing session device [%u:%u] in session %s", major(st.st_rdev), minor(st.st_rdev), s->id); From 5d5330a8e4c6f5926d74f1e0f4bfad2e6355235a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:32:07 +0100 Subject: [PATCH 02/10] logind: rework sd_eviocrevoke() Let's initialize static variables properly and get rid of redundant variables. --- src/login/logind-session-device.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index b1bac04036..0992f26d65 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -107,17 +107,15 @@ static int session_device_notify(SessionDevice *sd, enum SessionDeviceNotificati } static int sd_eviocrevoke(int fd) { - static bool warned; - int r; + static bool warned = false; assert(fd >= 0); - r = ioctl(fd, EVIOCREVOKE, NULL); - if (r < 0) { - r = -errno; - if (r == -EINVAL && !warned) { + if (ioctl(fd, EVIOCREVOKE, NULL) < 0) { + + if (errno == EINVAL && !warned) { + log_warning_errno(errno, "Kernel does not support evdev-revocation: %m"); warned = true; - log_warning("kernel does not support evdev-revocation"); } } From e38aa66426ad657b6a9adcbd041fab27e216594b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:33:05 +0100 Subject: [PATCH 03/10] logind: propagate the right error, don't make up ENOMEM --- src/login/logind-session-device.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 0992f26d65..30e29e122c 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -370,10 +370,8 @@ int session_device_new(Session *s, dev_t dev, bool open_device, SessionDevice ** goto error; r = hashmap_put(s->devices, &sd->dev, sd); - if (r < 0) { - r = -ENOMEM; + if (r < 0) goto error; - } if (open_device) { /* Open the device for the first time. We need a valid fd to pass back From d7ba71f4b44a10507c53f64834124545663eee17 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:33:20 +0100 Subject: [PATCH 04/10] logind: let's reduce one level of indentation --- src/login/logind-session-device.c | 35 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 30e29e122c..64162f6570 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -455,13 +455,14 @@ void session_device_resume_all(Session *s) { assert(s); HASHMAP_FOREACH(sd, s->devices, i) { - if (!sd->active) { - if (session_device_start(sd) < 0) - continue; - if (session_device_save(sd) < 0) - continue; - session_device_notify(sd, SESSION_DEVICE_RESUME); - } + if (sd->active) + continue; + + if (session_device_start(sd) < 0) + continue; + if (session_device_save(sd) < 0) + continue; + session_device_notify(sd, SESSION_DEVICE_RESUME); } } @@ -472,25 +473,27 @@ void session_device_pause_all(Session *s) { assert(s); HASHMAP_FOREACH(sd, s->devices, i) { - if (sd->active) { - session_device_stop(sd); - session_device_notify(sd, SESSION_DEVICE_PAUSE); - } + if (!sd->active) + continue; + + session_device_stop(sd); + session_device_notify(sd, SESSION_DEVICE_PAUSE); } } unsigned int session_device_try_pause_all(Session *s) { + unsigned num_pending = 0; SessionDevice *sd; Iterator i; - unsigned int num_pending = 0; assert(s); HASHMAP_FOREACH(sd, s->devices, i) { - if (sd->active) { - session_device_notify(sd, SESSION_DEVICE_TRY_PAUSE); - ++num_pending; - } + if (!sd->active) + continue; + + session_device_notify(sd, SESSION_DEVICE_TRY_PAUSE); + num_pending++; } return num_pending; From 4c9cb12c0536503979f44d04491ea7bbe118a4cc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:33:51 +0100 Subject: [PATCH 05/10] logind: fd 0 is a valid fd --- src/libsystemd-network/sd-dhcp-server.c | 2 +- src/login/logind-session-device.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c index 907b72391b..d64a47d2fc 100644 --- a/src/libsystemd-network/sd-dhcp-server.c +++ b/src/libsystemd-network/sd-dhcp-server.c @@ -316,7 +316,7 @@ static int dhcp_server_send_udp(sd_dhcp_server *server, be32_t destination, int r; assert(server); - assert(server->fd > 0); + assert(server->fd >= 0); assert(message); assert(len > sizeof(DHCPMessage)); diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index 64162f6570..f160af17ba 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -527,7 +527,7 @@ int session_device_save(SessionDevice *sd) { } void session_device_attach_fd(SessionDevice *sd, int fd, bool active) { - assert(fd > 0); + assert(fd >= 0); assert(sd); assert(sd->fd < 0); assert(!sd->active); From 0410444446c84a759a8f2d0917710849fc91384c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:34:13 +0100 Subject: [PATCH 06/10] logind: let's pack a few struct fields we can pack --- src/login/logind-session-device.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/login/logind-session-device.h b/src/login/logind-session-device.h index a1cf17af92..a9ead7bdca 100644 --- a/src/login/logind-session-device.h +++ b/src/login/logind-session-device.h @@ -39,9 +39,9 @@ struct SessionDevice { dev_t dev; char *node; int fd; - bool active; - DeviceType type; - bool pushed_fd; + DeviceType type:3; + bool active:1; + bool pushed_fd:1; LIST_FIELDS(struct SessionDevice, sd_by_device); }; From 51ead3e3774aa9306d637723d92bbddf2258d2cb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:34:43 +0100 Subject: [PATCH 07/10] logind: check file is device node before using .st_rdev --- src/login/logind.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/login/logind.c b/src/login/logind.c index 875d467cbb..5220861c1d 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -453,6 +453,12 @@ static int manager_attach_fds(Manager *m) { continue; } + if (!S_ISCHR(st.st_mode) && !S_ISBLK(st.st_mode)) { + log_debug("Device fd doesn't actually point to device node: %m"); + close_nointr(fd); + continue; + } + sd = hashmap_get(s->devices, &st.st_rdev); if (!sd) { /* Weird, we got an fd for a session device which wasn't From 4d219f5343b1924e7c519c2c178aeb5d1a5ab924 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:34:49 +0100 Subject: [PATCH 08/10] logind: make sure we don't trip up on half-initialized session devices Fixes: #8035 --- src/login/logind-session-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/login/logind-session-device.c b/src/login/logind-session-device.c index f160af17ba..65b4bb849b 100644 --- a/src/login/logind-session-device.c +++ b/src/login/logind-session-device.c @@ -420,7 +420,7 @@ void session_device_free(SessionDevice *sd) { session_device_stop(sd); session_device_notify(sd, SESSION_DEVICE_RELEASE); - close_nointr(sd->fd); + safe_close(sd->fd); LIST_REMOVE(sd_by_device, sd->device->session_devices, sd); From 3ccf323dfd70eaf010ad51f416af9414ae24486f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:45:28 +0100 Subject: [PATCH 09/10] journal-upload: make use of safe_close() where appropriate --- src/journal-remote/journal-upload.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/journal-remote/journal-upload.c b/src/journal-remote/journal-upload.c index 0b74ca98a7..3094785618 100644 --- a/src/journal-remote/journal-upload.c +++ b/src/journal-remote/journal-upload.c @@ -328,9 +328,7 @@ static size_t fd_input_callback(void *buf, size_t size, size_t nmemb, void *user static void close_fd_input(Uploader *u) { assert(u); - if (u->input >= 0) - close_nointr(u->input); - u->input = -1; + u->input = safe_close(u->input); u->timeout = 0; } From 4cbbc2a2a4257fd83d0db17ce46750b02ce73b53 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Feb 2018 18:45:45 +0100 Subject: [PATCH 10/10] sd-login: make use of _cleanup_close_ where possible --- src/libsystemd/sd-login/sd-login.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-login/sd-login.c b/src/libsystemd/sd-login/sd-login.c index 69572d1a51..6601bcd6be 100644 --- a/src/libsystemd/sd-login/sd-login.c +++ b/src/libsystemd/sd-login/sd-login.c @@ -990,8 +990,9 @@ static inline sd_login_monitor* FD_TO_MONITOR(int fd) { } _public_ int sd_login_monitor_new(const char *category, sd_login_monitor **m) { - int fd, k; + _cleanup_close_ int fd = -1; bool good = false; + int k; assert_return(m, -EINVAL); @@ -1001,50 +1002,42 @@ _public_ int sd_login_monitor_new(const char *category, sd_login_monitor **m) { if (!category || streq(category, "seat")) { k = inotify_add_watch(fd, "/run/systemd/seats/", IN_MOVED_TO|IN_DELETE); - if (k < 0) { - safe_close(fd); + if (k < 0) return -errno; - } good = true; } if (!category || streq(category, "session")) { k = inotify_add_watch(fd, "/run/systemd/sessions/", IN_MOVED_TO|IN_DELETE); - if (k < 0) { - safe_close(fd); + if (k < 0) return -errno; - } good = true; } if (!category || streq(category, "uid")) { k = inotify_add_watch(fd, "/run/systemd/users/", IN_MOVED_TO|IN_DELETE); - if (k < 0) { - safe_close(fd); + if (k < 0) return -errno; - } good = true; } if (!category || streq(category, "machine")) { k = inotify_add_watch(fd, "/run/systemd/machines/", IN_MOVED_TO|IN_DELETE); - if (k < 0) { - safe_close(fd); + if (k < 0) return -errno; - } good = true; } - if (!good) { - close_nointr(fd); + if (!good) return -EINVAL; - } *m = FD_TO_MONITOR(fd); + fd = -1; + return 0; }