Merge pull request #32684 from YHNdnzj/pr-followups

Follow-ups for recently merged PRs

src/basic/mkdir.c

@@ -209,6 +209,8 @@ int mkdir_p_root_full(const char *root, const char *p, uid_t uid, gid_t gid, mod
         _cleanup_close_ int dfd = -EBADF;
         int r;
 
+        assert(p);
+
         r = path_extract_directory(p, &pp);
         if (r == -EDESTADDRREQ) {
                 /* only fname is passed, no prefix to operate on */
@@ -226,7 +228,7 @@ int mkdir_p_root_full(const char *root, const char *p, uid_t uid, gid_t gid, mod
                 if (r < 0)
                         return r;
 
-                dfd = chase_and_open(pp, root, CHASE_PREFIX_ROOT, O_RDONLY|O_CLOEXEC|O_DIRECTORY, NULL);
+                dfd = chase_and_open(pp, root, CHASE_PREFIX_ROOT, O_CLOEXEC|O_DIRECTORY, NULL);
                 if (dfd < 0)
                         return dfd;
         }
@@ -241,25 +243,22 @@ int mkdir_p_root_full(const char *root, const char *p, uid_t uid, gid_t gid, mod
                 r = btrfs_subvol_make_fallback(dfd, bn, m);
         else
                 r = RET_NERRNO(mkdirat(dfd, bn, m));
-        if (r < 0) {
-                if (r == -EEXIST)
-                        return 0;
-
+        if (r == -EEXIST)
+                return 0;
+        if (r < 0)
                 return r;
-        }
 
         if (ts == USEC_INFINITY && !uid_is_valid(uid) && !gid_is_valid(gid))
                 return 1;
 
-        _cleanup_close_ int nfd = -EBADF;
-        nfd = openat(dfd, bn, O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_NOFOLLOW);
+        _cleanup_close_ int nfd = openat(dfd, bn, O_CLOEXEC|O_DIRECTORY|O_NOFOLLOW);
         if (nfd < 0)
                 return -errno;
 
         if (ts != USEC_INFINITY) {
                 struct timespec tspec;
+                timespec_store(&tspec, ts);
 
-                timespec_store_nsec(&tspec, ts);
                 if (futimens(dfd, (const struct timespec[2]) { { .tv_nsec = UTIME_OMIT }, tspec }) < 0)
                         return -errno;
 

src/core/execute.c

@@ -353,20 +353,18 @@ static void log_command_line(Unit *unit, const char *msg, const char *executable
 
 static int exec_context_load_environment(const Unit *unit, const ExecContext *c, char ***l);
 
-int exec_spawn(Unit *unit,
-               ExecCommand *command,
-               const ExecContext *context,
-               ExecParameters *params,
-               ExecRuntime *runtime,
-               const CGroupContext *cgroup_context,
-               PidRef *ret) {
+int exec_spawn(
+                Unit *unit,
+                ExecCommand *command,
+                const ExecContext *context,
+                ExecParameters *params,
+                ExecRuntime *runtime,
+                const CGroupContext *cgroup_context,
+                PidRef *ret) {
 
-        char serialization_fd_number[DECIMAL_STR_MAX(int) + 1];
         _cleanup_free_ char *subcgroup_path = NULL, *max_log_levels = NULL, *executor_path = NULL;
-        _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
         _cleanup_fdset_free_ FDSet *fdset = NULL;
         _cleanup_fclose_ FILE *f = NULL;
-        dual_timestamp start_timestamp;
         int r;
 
         assert(unit);
@@ -445,8 +443,12 @@ int exec_spawn(Unit *unit,
         if (r < 0)
                 return log_unit_error_errno(unit, r, "Failed to get executor path from fd: %m");
 
+        char serialization_fd_number[DECIMAL_STR_MAX(int)];
         xsprintf(serialization_fd_number, "%i", fileno(f));
 
+        _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
+        dual_timestamp start_timestamp;
+
         /* Record the start timestamp before we fork so that it is guaranteed to be earlier than the
          * handoff timestamp. */
         dual_timestamp_now(&start_timestamp);

src/core/execute.h

@@ -479,13 +479,14 @@ struct ExecParameters {
 #include "unit.h"
 #include "dynamic-user.h"
 
-int exec_spawn(Unit *unit,
-               ExecCommand *command,
-               const ExecContext *context,
-               ExecParameters *exec_params,
-               ExecRuntime *runtime,
-               const CGroupContext *cgroup_context,
-               PidRef *ret);
+int exec_spawn(
+                Unit *unit,
+                ExecCommand *command,
+                const ExecContext *context,
+                ExecParameters *exec_params,
+                ExecRuntime *runtime,
+                const CGroupContext *cgroup_context,
+                PidRef *ret);
 
 void exec_command_done(ExecCommand *c);
 void exec_command_done_array(ExecCommand *c, size_t n);

src/core/service.c

@@ -2413,35 +2413,38 @@ static void service_enter_start(Service *s) {
                 goto fail;
         }
 
-        if (IN_SET(s->type, SERVICE_SIMPLE, SERVICE_IDLE)) {
-                /* For simple services we immediately start
-                 * the START_POST binaries. */
+        assert(pidref.pid == c->exec_status.pid);
 
+        switch (s->type) {
+
+        case SERVICE_SIMPLE:
+        case SERVICE_IDLE:
+                /* For simple services we immediately start the START_POST binaries. */
                 (void) service_set_main_pidref(s, TAKE_PIDREF(pidref), &c->exec_status.start_timestamp);
-                service_enter_start_post(s);
-
-        } else  if (s->type == SERVICE_FORKING) {
-
-                /* For forking services we wait until the start
-                 * process exited. */
+                return service_enter_start_post(s);
 
+        case SERVICE_FORKING:
+                /* For forking services we wait until the start process exited. */
                 pidref_done(&s->control_pid);
                 s->control_pid = TAKE_PIDREF(pidref);
-                service_set_state(s, SERVICE_START);
-
-        } else if (IN_SET(s->type, SERVICE_ONESHOT, SERVICE_DBUS, SERVICE_NOTIFY, SERVICE_NOTIFY_RELOAD, SERVICE_EXEC)) {
-
-                /* For oneshot services we wait until the start process exited, too, but it is our main process. */
-
-                /* For D-Bus services we know the main pid right away, but wait for the bus name to appear on the
-                 * bus. 'notify' and 'exec' services are similar. */
+                return service_set_state(s, SERVICE_START);
 
+        case SERVICE_ONESHOT: /* For oneshot services we wait until the start process exited, too, but it is our main process. */
+        case SERVICE_EXEC:
+        case SERVICE_DBUS:
+        case SERVICE_NOTIFY:
+        case SERVICE_NOTIFY_RELOAD:
+                /* For D-Bus services we know the main pid right away, but wait for the bus name to appear
+                 * on the bus. 'notify' and 'exec' services wait for readiness notification and EOF
+                 * on exec_fd, respectively. */
                 (void) service_set_main_pidref(s, TAKE_PIDREF(pidref), &c->exec_status.start_timestamp);
-                service_set_state(s, SERVICE_START);
-        } else
-                assert_not_reached();
+                return service_set_state(s, SERVICE_START);
 
-        return;
+        default:
+                ;
+        }
+
+        assert_not_reached();
 
 fail:
         service_enter_signal(s, SERVICE_STOP_SIGTERM, SERVICE_FAILURE_RESOURCES);

src/core/unit.c

@@ -1311,33 +1311,25 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c) {
          * is run first. */
 
         if (c->log_namespace) {
-                _cleanup_free_ char *unit = NULL;
+                FOREACH_STRING(s, "systemd-journald", "systemd-journald-varlink") {
+                        _cleanup_free_ char *socket_unit = NULL;
 
-                r = unit_name_build_from_type("systemd-journald", c->log_namespace, UNIT_SOCKET, &unit);
+                        r = unit_name_build_from_type(s, c->log_namespace, UNIT_SOCKET, &socket_unit);
+                        if (r < 0)
+                                return r;
+
+                        r = unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_REQUIRES, socket_unit, true, UNIT_DEPENDENCY_FILE);
+                        if (r < 0)
+                                return r;
+                }
+
+                _cleanup_free_ char *sync_unit = NULL;
+
+                r = unit_name_build_from_type("systemd-journald-sync", c->log_namespace, UNIT_SERVICE, &sync_unit);
                 if (r < 0)
                         return r;
 
-                r = unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_REQUIRES, unit, true, UNIT_DEPENDENCY_FILE);
-                if (r < 0)
-                        return r;
-
-                unit = mfree(unit);
-
-                r = unit_name_build_from_type("systemd-journald-varlink", c->log_namespace, UNIT_SOCKET, &unit);
-                if (r < 0)
-                        return r;
-
-                r = unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_REQUIRES, unit, true, UNIT_DEPENDENCY_FILE);
-                if (r < 0)
-                        return r;
-
-                unit = mfree(unit);
-
-                r = unit_name_build_from_type("systemd-journald-sync", c->log_namespace, UNIT_SERVICE, &unit);
-                if (r < 0)
-                        return r;
-
-                r = unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_REQUIRES, unit, true, UNIT_DEPENDENCY_FILE);
+                r = unit_add_two_dependencies_by_name(u, UNIT_AFTER, UNIT_REQUIRES, sync_unit, true, UNIT_DEPENDENCY_FILE);
                 if (r < 0)
                         return r;
         } else {

src/partition/repart.c

@@ -4898,7 +4898,6 @@ static int do_copy_files(Context *context, Partition *p, const char *root) {
                                                        strempty(arg_copy_source), *source, strempty(root), *target);
                 } else {
                         _cleanup_free_ char *dn = NULL, *fn = NULL;
-                        struct timespec tspec;
 
                         /* We are looking at a regular file */
 
@@ -4933,10 +4932,13 @@ static int do_copy_files(Context *context, Partition *p, const char *root) {
                         (void) copy_access(sfd, tfd);
                         (void) copy_times(sfd, tfd, 0);
 
-                        timespec_store_nsec(&tspec, ts);
+                        if (ts != USEC_INFINITY) {
+                                struct timespec tspec;
+                                timespec_store(&tspec, ts);
 
-                        if (ts != USEC_INFINITY && futimens(pfd, (const struct timespec[2]) { { .tv_nsec = UTIME_OMIT }, tspec }) < 0)
-                                return -errno;
+                                if (futimens(pfd, (const struct timespec[2]) { { .tv_nsec = UTIME_OMIT }, tspec }) < 0)
+                                        return -errno;
+                        }
                 }
         }
 

src/test/test-mkdir.c

@@ -143,26 +143,26 @@ TEST(mkdir_p_root_full) {
         _cleanup_free_ char *p = NULL;
         struct stat st;
 
-        assert_se(mkdtemp_malloc("/tmp/test-mkdir-XXXXXX", &tmp) >= 0);
+        ASSERT_OK(mkdtemp_malloc("/tmp/test-mkdir-XXXXXX", &tmp));
 
-        assert_se(p = path_join(tmp, "foo"));
-        assert_se(mkdir_p_root_full(tmp, "/foo", UID_INVALID, GID_INVALID, 0755, 1234, NULL) >= 0);
-        assert_se(is_dir(p, false) > 0);
-        assert_se(is_dir(p, true) > 0);
-        assert_se(stat(p, &st) >= 0);
-        assert_se(st.st_mtim.tv_nsec == 1234);
-        assert_se(st.st_atim.tv_nsec == 1234);
+        ASSERT_NOT_NULL(p = path_join(tmp, "foo"));
+        ASSERT_OK(mkdir_p_root_full(tmp, "/foo", UID_INVALID, GID_INVALID, 0755, 2 * USEC_PER_SEC, NULL));
+        ASSERT_GT(is_dir(p, false), 0);
+        ASSERT_GT(is_dir(p, true), 0);
+        ASSERT_OK_ERRNO(stat(p, &st));
+        ASSERT_EQ(st.st_mtim.tv_sec, 2);
+        ASSERT_EQ(st.st_atim.tv_sec, 2);
 
         p = mfree(p);
-        assert_se(p = path_join(tmp, "dir-not-exists/foo"));
-        assert_se(mkdir_p_root_full(tmp, "/dir-not-exists/foo", UID_INVALID, GID_INVALID, 0755, 5678, NULL) >= 0);
-        assert_se(is_dir(p, false) > 0);
-        assert_se(is_dir(p, true) > 0);
+        ASSERT_NOT_NULL(p = path_join(tmp, "dir-not-exists/foo"));
+        ASSERT_OK(mkdir_p_root_full(NULL, p, UID_INVALID, GID_INVALID, 0755, 90 * USEC_PER_HOUR, NULL));
+        ASSERT_GT(is_dir(p, false), 0);
+        ASSERT_GT(is_dir(p, true), 0);
         p = mfree(p);
-        assert_se(p = path_join(tmp, "dir-not-exists"));
-        assert_se(stat(p, &st) >= 0);
-        assert_se(st.st_mtim.tv_nsec == 5678);
-        assert_se(st.st_atim.tv_nsec == 5678);
+        ASSERT_NOT_NULL(p = path_join(tmp, "dir-not-exists"));
+        ASSERT_OK_ERRNO(stat(p, &st));
+        ASSERT_EQ(st.st_mtim.tv_sec, 90 * 60 * 60);
+        ASSERT_EQ(st.st_atim.tv_sec, 90 * 60 * 60);
 }
 
 DEFINE_TEST_MAIN(LOG_DEBUG);