From 192aee3cae72bf4ca36c849061894e07250b93b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Jan 2020 10:04:30 +0100 Subject: [PATCH 1/4] shared/user-record-nss: shorten code a bit free_and_strdup() already does comparison internally. --- src/shared/user-record-nss.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/src/shared/user-record-nss.c b/src/shared/user-record-nss.c index 11c085d63d..cf273653de 100644 --- a/src/shared/user-record-nss.c +++ b/src/shared/user-record-nss.c @@ -31,29 +31,18 @@ int nss_passwd_to_user_record( if (r < 0) return r; - if (isempty(pwd->pw_gecos) || streq_ptr(pwd->pw_gecos, hr->user_name)) - hr->real_name = mfree(hr->real_name); - else { - r = free_and_strdup(&hr->real_name, pwd->pw_gecos); - if (r < 0) - return r; - } + r = free_and_strdup(&hr->real_name, + streq_ptr(pwd->pw_gecos, hr->user_name) ? NULL : empty_to_null(pwd->pw_gecos)); + if (r < 0) + return r; - if (isempty(pwd->pw_dir)) - hr->home_directory = mfree(hr->home_directory); - else { - r = free_and_strdup(&hr->home_directory, pwd->pw_dir); - if (r < 0) - return r; - } + r = free_and_strdup(&hr->home_directory, empty_to_null(pwd->pw_dir)); + if (r < 0) + return r; - if (isempty(pwd->pw_shell)) - hr->shell = mfree(hr->shell); - else { - r = free_and_strdup(&hr->shell, pwd->pw_shell); - if (r < 0) - return r; - } + r = free_and_strdup(&hr->shell, empty_to_null(pwd->pw_shell)); + if (r < 0) + return r; hr->uid = pwd->pw_uid; hr->gid = pwd->pw_gid; From ddee3ada467d25e77fef3434fbed7191ff0c1a34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Jan 2020 10:37:18 +0100 Subject: [PATCH 2/4] shared/user-record-nss: use macro to avoid repeats It's easier to read when each field is intialized in exactly one place. --- src/shared/user-record-nss.c | 109 +++++++++++++++-------------------- 1 file changed, 46 insertions(+), 63 deletions(-) diff --git a/src/shared/user-record-nss.c b/src/shared/user-record-nss.c index cf273653de..0ff6d17117 100644 --- a/src/shared/user-record-nss.c +++ b/src/shared/user-record-nss.c @@ -6,6 +6,9 @@ #include "strv.h" #include "user-record-nss.h" +#define SET_IF(field, condition, value, fallback) \ + field = (condition) ? (value) : (fallback) + int nss_passwd_to_user_record( const struct passwd *pwd, const struct spwd *spwd, @@ -47,70 +50,50 @@ int nss_passwd_to_user_record( hr->uid = pwd->pw_uid; hr->gid = pwd->pw_gid; - if (spwd) { - if (hashed_password_valid(spwd->sp_pwdp)) { - strv_free_erase(hr->hashed_password); - hr->hashed_password = strv_new(spwd->sp_pwdp); - if (!hr->hashed_password) - return -ENOMEM; - } else - hr->hashed_password = strv_free_erase(hr->hashed_password); - - /* shadow-utils suggests using "chage -E 0" (or -E 1, depending on which man page you check) - * for locking a whole account, hence check for that. Note that it also defines a way to lock - * just a password instead of the whole account, but that's mostly pointless in times of - * password-less authorization, hence let's not bother. */ - - if (spwd->sp_expire >= 0) - hr->locked = spwd->sp_expire <= 1; - else - hr->locked = -1; - - if (spwd->sp_expire > 1 && (uint64_t) spwd->sp_expire < (UINT64_MAX-1)/USEC_PER_DAY) - hr->not_after_usec = spwd->sp_expire * USEC_PER_DAY; - else - hr->not_after_usec = UINT64_MAX; - - if (spwd->sp_lstchg >= 0) - hr->password_change_now = spwd->sp_lstchg == 0; - else - hr->password_change_now = -1; - - if (spwd->sp_lstchg > 0 && (uint64_t) spwd->sp_lstchg <= (UINT64_MAX-1)/USEC_PER_DAY) - hr->last_password_change_usec = spwd->sp_lstchg * USEC_PER_DAY; - else - hr->last_password_change_usec = UINT64_MAX; - - if (spwd->sp_min > 0 && (uint64_t) spwd->sp_min <= (UINT64_MAX-1)/USEC_PER_DAY) - hr->password_change_min_usec = spwd->sp_min * USEC_PER_DAY; - else - hr->password_change_min_usec = UINT64_MAX; - - if (spwd->sp_max > 0 && (uint64_t) spwd->sp_max <= (UINT64_MAX-1)/USEC_PER_DAY) - hr->password_change_max_usec = spwd->sp_max * USEC_PER_DAY; - else - hr->password_change_max_usec = UINT64_MAX; - - if (spwd->sp_warn > 0 && (uint64_t) spwd->sp_warn <= (UINT64_MAX-1)/USEC_PER_DAY) - hr->password_change_warn_usec = spwd->sp_warn * USEC_PER_DAY; - else - hr->password_change_warn_usec = UINT64_MAX; - - if (spwd->sp_inact > 0 && (uint64_t) spwd->sp_inact <= (UINT64_MAX-1)/USEC_PER_DAY) - hr->password_change_inactive_usec = spwd->sp_inact * USEC_PER_DAY; - else - hr->password_change_inactive_usec = UINT64_MAX; - } else { + if (spwd && hashed_password_valid(spwd->sp_pwdp)) { + strv_free_erase(hr->hashed_password); + hr->hashed_password = strv_new(spwd->sp_pwdp); + if (!hr->hashed_password) + return -ENOMEM; + } else hr->hashed_password = strv_free_erase(hr->hashed_password); - hr->locked = -1; - hr->not_after_usec = UINT64_MAX; - hr->password_change_now = -1, - hr->last_password_change_usec = UINT64_MAX; - hr->password_change_min_usec = UINT64_MAX; - hr->password_change_max_usec = UINT64_MAX; - hr->password_change_warn_usec = UINT64_MAX; - hr->password_change_inactive_usec = UINT64_MAX; - } + + /* shadow-utils suggests using "chage -E 0" (or -E 1, depending on which man page you check) + * for locking a whole account, hence check for that. Note that it also defines a way to lock + * just a password instead of the whole account, but that's mostly pointless in times of + * password-less authorization, hence let's not bother. */ + + SET_IF(hr->locked, + spwd && spwd->sp_expire >= 0, + spwd->sp_expire <= 1, -1); + + SET_IF(hr->not_after_usec, + spwd && spwd->sp_expire > 1 && (uint64_t) spwd->sp_expire < (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_expire * USEC_PER_DAY, UINT64_MAX); + + SET_IF(hr->password_change_now, + spwd && spwd->sp_lstchg >= 0, + spwd->sp_lstchg == 0, -1); + + SET_IF(hr->last_password_change_usec, + spwd && spwd->sp_lstchg > 0 && (uint64_t) spwd->sp_lstchg <= (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_lstchg * USEC_PER_DAY, UINT64_MAX); + + SET_IF(hr->password_change_min_usec, + spwd && spwd->sp_min > 0 && (uint64_t) spwd->sp_min <= (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_min * USEC_PER_DAY, UINT64_MAX); + + SET_IF(hr->password_change_max_usec, + spwd && spwd->sp_max > 0 && (uint64_t) spwd->sp_max <= (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_max * USEC_PER_DAY, UINT64_MAX); + + SET_IF(hr->password_change_warn_usec, + spwd && spwd->sp_warn > 0 && (uint64_t) spwd->sp_warn <= (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_warn * USEC_PER_DAY, UINT64_MAX); + + SET_IF(hr->password_change_inactive_usec, + spwd && spwd->sp_inact > 0 && (uint64_t) spwd->sp_inact <= (UINT64_MAX-1)/USEC_PER_DAY, + spwd->sp_inact * USEC_PER_DAY, UINT64_MAX); hr->json = json_variant_unref(hr->json); r = json_build(&hr->json, JSON_BUILD_OBJECT( From b0a94df9631d721699ba90fc7e692b0f51a05770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Jan 2020 11:37:49 +0100 Subject: [PATCH 3/4] logind: use loop instead of repeated code https://github.com/systemd/systemd/pull/14096#discussion_r350953689 --- src/login/logind-user.c | 44 +++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/src/login/logind-user.c b/src/login/logind-user.c index fdc6448a65..9ceb33cde9 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -405,35 +405,23 @@ static int user_update_slice(User *u) { if (r < 0) return bus_log_create_error(r); - if (u->user_record->tasks_max != UINT64_MAX) { - r = sd_bus_message_append(m, "(sv)", "TasksMax", "t", u->user_record->tasks_max); - if (r < 0) - return bus_log_create_error(r); - } + const struct { + const char *name; + uint64_t value; + } settings[] = { + { "TasksMax", u->user_record->tasks_max }, + { "MemoryMax", u->user_record->memory_max }, + { "MemoryHigh", u->user_record->memory_high }, + { "CPUWeight", u->user_record->cpu_weight }, + { "IOWeight", u->user_record->io_weight }, + }; - if (u->user_record->memory_max != UINT64_MAX) { - r = sd_bus_message_append(m, "(sv)", "MemoryMax", "t", u->user_record->memory_max); - if (r < 0) - return bus_log_create_error(r); - } - - if (u->user_record->memory_high != UINT64_MAX) { - r = sd_bus_message_append(m, "(sv)", "MemoryHigh", "t", u->user_record->memory_high); - if (r < 0) - return bus_log_create_error(r); - } - - if (u->user_record->cpu_weight != UINT64_MAX) { - r = sd_bus_message_append(m, "(sv)", "CPUWeight", "t", u->user_record->cpu_weight); - if (r < 0) - return bus_log_create_error(r); - } - - if (u->user_record->io_weight != UINT64_MAX) { - r = sd_bus_message_append(m, "(sv)", "IOWeight", "t", u->user_record->io_weight); - if (r < 0) - return bus_log_create_error(r); - } + for (size_t i = 0; i < ELEMENTSOF(settings); i++) + if (settings[i].value != UINT64_MAX) { + r = sd_bus_message_append(m, "(sv)", settings[i].name, "t", settings[i].value); + if (r < 0) + return bus_log_create_error(r); + } r = sd_bus_message_close_container(m); if (r < 0) From 4ca739e20a09300f6cd11ce7d0c776b27a6d49fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 15 Jan 2020 11:42:40 +0100 Subject: [PATCH 4/4] core: reduce indentation a bit --- src/core/core-varlink.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/core/core-varlink.c b/src/core/core-varlink.c index 628b9ccaaa..8a2984a2d4 100644 --- a/src/core/core-varlink.c +++ b/src/core/core-varlink.c @@ -21,16 +21,16 @@ static int build_user_json(const char *user_name, uid_t uid, JsonVariant **ret) assert(ret); return json_build(ret, JSON_BUILD_OBJECT( - JSON_BUILD_PAIR("record", JSON_BUILD_OBJECT( - JSON_BUILD_PAIR("userName", JSON_BUILD_STRING(user_name)), - JSON_BUILD_PAIR("uid", JSON_BUILD_UNSIGNED(uid)), - JSON_BUILD_PAIR("gid", JSON_BUILD_UNSIGNED(uid)), - JSON_BUILD_PAIR("realName", JSON_BUILD_STRING("Dynamic User")), - JSON_BUILD_PAIR("homeDirectory", JSON_BUILD_STRING("/")), - JSON_BUILD_PAIR("shell", JSON_BUILD_STRING(NOLOGIN)), - JSON_BUILD_PAIR("locked", JSON_BUILD_BOOLEAN(true)), - JSON_BUILD_PAIR("service", JSON_BUILD_STRING("io.systemd.DynamicUser")), - JSON_BUILD_PAIR("disposition", JSON_BUILD_STRING("dynamic")))))); + JSON_BUILD_PAIR("record", JSON_BUILD_OBJECT( + JSON_BUILD_PAIR("userName", JSON_BUILD_STRING(user_name)), + JSON_BUILD_PAIR("uid", JSON_BUILD_UNSIGNED(uid)), + JSON_BUILD_PAIR("gid", JSON_BUILD_UNSIGNED(uid)), + JSON_BUILD_PAIR("realName", JSON_BUILD_STRING("Dynamic User")), + JSON_BUILD_PAIR("homeDirectory", JSON_BUILD_STRING("/")), + JSON_BUILD_PAIR("shell", JSON_BUILD_STRING(NOLOGIN)), + JSON_BUILD_PAIR("locked", JSON_BUILD_BOOLEAN(true)), + JSON_BUILD_PAIR("service", JSON_BUILD_STRING("io.systemd.DynamicUser")), + JSON_BUILD_PAIR("disposition", JSON_BUILD_STRING("dynamic")))))); } static bool user_match_lookup_parameters(LookupParameters *p, const char *name, uid_t uid) { @@ -134,12 +134,12 @@ static int build_group_json(const char *group_name, gid_t gid, JsonVariant **ret assert(ret); return json_build(ret, JSON_BUILD_OBJECT( - JSON_BUILD_PAIR("record", JSON_BUILD_OBJECT( - JSON_BUILD_PAIR("groupName", JSON_BUILD_STRING(group_name)), - JSON_BUILD_PAIR("gid", JSON_BUILD_UNSIGNED(gid)), - JSON_BUILD_PAIR("service", JSON_BUILD_STRING("io.systemd.DynamicUser")), - JSON_BUILD_PAIR("disposition", JSON_BUILD_STRING("dynamic")))))); -} + JSON_BUILD_PAIR("record", JSON_BUILD_OBJECT( + JSON_BUILD_PAIR("groupName", JSON_BUILD_STRING(group_name)), + JSON_BUILD_PAIR("gid", JSON_BUILD_UNSIGNED(gid)), + JSON_BUILD_PAIR("service", JSON_BUILD_STRING("io.systemd.DynamicUser")), + JSON_BUILD_PAIR("disposition", JSON_BUILD_STRING("dynamic")))))); + } static bool group_match_lookup_parameters(LookupParameters *p, const char *name, gid_t gid) { assert(p);