Merge pull request #22071 from keszybz/xdg-autostart-logs

Improve logs in systemd-xdg-autostart-generator

docs/DESKTOP_ENVIRONMENTS.md

@@ -97,13 +97,13 @@ to create appropriate units for the autostart directory
 (`systemd-xdg-autostart-generator`).
 Desktop Environments can opt-in to using this by starting
 `xdg-desktop-autostart.target`. The systemd generator correctly handles
-`OnlyShowIn=` and `NotShowin=`. It also handles the KDE and GNOME specific
-`X-KDE-autostart-condition=` and `AutostartCondition=` by using desktop
-environment provided binaries in an `ExecCondition=` line.
+`OnlyShowIn=` and `NotShowIn=`. It also handles the KDE and GNOME specific
+`X-KDE-autostart-condition=` and `AutostartCondition=` by using desktop-environment-provided
+binaries in an `ExecCondition=` line.
 
 However, this generator is somewhat limited in what it supports. For example,
 all generated units will have `After=graphical-session.target` set on them,
-it may therefore not be useful to start session services.
+and therefore may not be useful to start session services.
 
 Desktop files can be marked to be explicitly excluded from the generator using the line
 `X-systemd-skip=true`. This should be set if an application provides its own

src/shared/rm-rf.c

@@ -52,7 +52,6 @@ static int patch_dirfd_mode(
 }
 
 int unlinkat_harder(int dfd, const char *filename, int unlink_flags, RemoveFlags remove_flags) {
-
         mode_t old_mode;
         int r;
 
@@ -116,15 +115,16 @@ int fstatat_harder(int dfd,
         return 0;
 }
 
-static int rm_rf_children_inner(
+static int rm_rf_inner_child(
                 int fd,
                 const char *fname,
                 int is_dir,
                 RemoveFlags flags,
-                const struct stat *root_dev) {
+                const struct stat *root_dev,
+                bool allow_recursion) {
 
         struct stat st;
-        int r;
+        int r, q = 0;
 
         assert(fd >= 0);
         assert(fname);
@@ -141,10 +141,7 @@ static int rm_rf_children_inner(
         }
 
         if (is_dir) {
-                _cleanup_close_ int subdir_fd = -1;
-                int q;
-
-                /* if root_dev is set, remove subdirectories only if device is same */
+                /* If root_dev is set, remove subdirectories only if device is same */
                 if (root_dev && st.st_dev != root_dev->st_dev)
                         return 0;
 
@@ -156,7 +153,6 @@ static int rm_rf_children_inner(
                         return 0;
 
                 if ((flags & REMOVE_SUBVOLUME) && btrfs_might_be_subvol(&st)) {
-
                         /* This could be a subvolume, try to remove it */
 
                         r = btrfs_subvol_remove_fd(fd, fname, BTRFS_REMOVE_RECURSIVE|BTRFS_REMOVE_QUOTA);
@@ -170,31 +166,40 @@ static int rm_rf_children_inner(
                                 return 1;
                 }
 
-                subdir_fd = openat(fd, fname, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME);
+                if (!allow_recursion)
+                        return -EISDIR;
+
+                int subdir_fd = openat(fd, fname, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME);
                 if (subdir_fd < 0)
                         return -errno;
 
                 /* We pass REMOVE_PHYSICAL here, to avoid doing the fstatfs() to check the file system type
                  * again for each directory */
-                q = rm_rf_children(TAKE_FD(subdir_fd), flags | REMOVE_PHYSICAL, root_dev);
+                q = rm_rf_children(subdir_fd, flags | REMOVE_PHYSICAL, root_dev);
 
-                r = unlinkat_harder(fd, fname, AT_REMOVEDIR, flags);
+        } else if (flags & REMOVE_ONLY_DIRECTORIES)
+                return 0;
+
+        r = unlinkat_harder(fd, fname, is_dir ? AT_REMOVEDIR : 0, flags);
         if (r < 0)
                 return r;
         if (q < 0)
                 return q;
-
-                return 1;
-
-        } else if (!(flags & REMOVE_ONLY_DIRECTORIES)) {
-                r = unlinkat_harder(fd, fname, 0, flags);
-                if (r < 0)
-                        return r;
-
         return 1;
 }
 
-        return 0;
+typedef struct TodoEntry {
+        DIR *dir;         /* A directory that we were operating on. */
+        char *dirname;    /* The filename of that directory itself. */
+} TodoEntry;
+
+static void free_todo_entries(TodoEntry **todos) {
+        for (TodoEntry *x = *todos; x && x->dir; x++) {
+                closedir(x->dir);
+                free(x->dirname);
+        }
+
+        freep(todos);
 }
 
 int rm_rf_children(
@@ -202,30 +207,56 @@ int rm_rf_children(
                 RemoveFlags flags,
                 const struct stat *root_dev) {
 
-        _cleanup_closedir_ DIR *d = NULL;
+        _cleanup_(free_todo_entries) TodoEntry *todos = NULL;
+        size_t n_todo = 0;
+        _cleanup_free_ char *dirname = NULL; /* Set when we are recursing and want to delete ourselves */
         int ret = 0, r;
 
+        /* Return the first error we run into, but nevertheless try to go on.
+         * The passed fd is closed in all cases, including on failure. */
+
+        for (;;) {  /* This loop corresponds to the directory nesting level. */
+                _cleanup_closedir_ DIR *d = NULL;
+
+                if (n_todo > 0) {
+                        /* We know that we are in recursion here, because n_todo is set.
+                         * We need to remove the inner directory we were operating on. */
+                        assert(dirname);
+                        r = unlinkat_harder(dirfd(todos[n_todo-1].dir), dirname, AT_REMOVEDIR, flags);
+                        if (r < 0 && r != -ENOENT && ret == 0)
+                                ret = r;
+                        dirname = mfree(dirname);
+
+                        /* And now let's back out one level up */
+                        n_todo --;
+                        d = TAKE_PTR(todos[n_todo].dir);
+                        dirname = TAKE_PTR(todos[n_todo].dirname);
+
+                        assert(d);
+                        fd = dirfd(d); /* Retrieve the file descriptor from the DIR object */
+                        assert(fd >= 0);
+                } else {
+        next_fd:
                         assert(fd >= 0);
-
-        /* This returns the first error we run into, but nevertheless tries to go on. This closes the passed
-         * fd, in all cases, including on failure. */
-
                         d = fdopendir(fd);
                         if (!d) {
                                 safe_close(fd);
                                 return -errno;
                         }
+                        fd = dirfd(d); /* We donated the fd to fdopendir(). Let's make sure we sure we have
+                                        * the right descriptor even if it were to internally invalidate the
+                                        * one we passed. */
 
                         if (!(flags & REMOVE_PHYSICAL)) {
                                 struct statfs sfs;
 
-                if (fstatfs(dirfd(d), &sfs) < 0)
+                                if (fstatfs(fd, &sfs) < 0)
                                         return -errno;
 
                                 if (is_physical_fs(&sfs)) {
-                        /* We refuse to clean physical file systems with this call, unless explicitly
-                         * requested. This is extra paranoia just to be sure we never ever remove non-state
-                         * data. */
+                                        /* We refuse to clean physical file systems with this call, unless
+                                         * explicitly requested. This is extra paranoia just to be sure we
+                                         * never ever remove non-state data. */
 
                                         _cleanup_free_ char *path = NULL;
 
@@ -235,6 +266,7 @@ int rm_rf_children(
                                                                strna(path));
                                 }
                         }
+                }
 
                 FOREACH_DIRENT_ALL(de, d, return -errno) {
                         int is_dir;
@@ -242,23 +274,47 @@ int rm_rf_children(
                         if (dot_or_dot_dot(de->d_name))
                                 continue;
 
-                is_dir =
-                        de->d_type == DT_UNKNOWN ? -1 :
-                        de->d_type == DT_DIR;
+                        is_dir = de->d_type == DT_UNKNOWN ? -1 : de->d_type == DT_DIR;
 
-                r = rm_rf_children_inner(dirfd(d), de->d_name, is_dir, flags, root_dev);
-                if (r < 0 && r != -ENOENT && ret == 0)
+                        r = rm_rf_inner_child(fd, de->d_name, is_dir, flags, root_dev, false);
+                        if (r == -EISDIR) {
+                                /* Push the current working state onto the todo list */
+
+                                 if (!GREEDY_REALLOC0(todos, n_todo + 2))
+                                         return log_oom();
+
+                                 _cleanup_free_ char *newdirname = strdup(de->d_name);
+                                 if (!newdirname)
+                                         return log_oom();
+
+                                 int newfd = openat(fd, de->d_name,
+                                                    O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME);
+                                 if (newfd >= 0) {
+                                         todos[n_todo++] = (TodoEntry) { TAKE_PTR(d), TAKE_PTR(dirname) };
+                                         fd = newfd;
+                                         dirname = TAKE_PTR(newdirname);
+
+                                         goto next_fd;
+
+                                 } else if (errno != -ENOENT && ret == 0)
+                                         ret = -errno;
+
+                        } else if (r < 0 && r != -ENOENT && ret == 0)
                                 ret = r;
                 }
 
-        if (FLAGS_SET(flags, REMOVE_SYNCFS) && syncfs(dirfd(d)) < 0 && ret >= 0)
+                if (FLAGS_SET(flags, REMOVE_SYNCFS) && syncfs(fd) < 0 && ret >= 0)
                         ret = -errno;
 
+                if (n_todo == 0)
+                        break;
+        }
+
         return ret;
 }
 
 int rm_rf(const char *path, RemoveFlags flags) {
-        int fd, r;
+        int fd, r, q = 0;
 
         assert(path);
 
@@ -290,18 +346,22 @@ int rm_rf(const char *path, RemoveFlags flags) {
         }
 
         fd = open(path, O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME);
-        if (fd < 0) {
+        if (fd >= 0) {
+                /* We have a dir */
+                r = rm_rf_children(fd, flags, NULL);
+
+                if (FLAGS_SET(flags, REMOVE_ROOT))
+                        q = RET_NERRNO(rmdir(path));
+        } else {
                 if (FLAGS_SET(flags, REMOVE_MISSING_OK) && errno == ENOENT)
                         return 0;
 
                 if (!IN_SET(errno, ENOTDIR, ELOOP))
                         return -errno;
 
-                if (FLAGS_SET(flags, REMOVE_ONLY_DIRECTORIES))
+                if (FLAGS_SET(flags, REMOVE_ONLY_DIRECTORIES) || !FLAGS_SET(flags, REMOVE_ROOT))
                         return 0;
 
-                if (FLAGS_SET(flags, REMOVE_ROOT)) {
-
                 if (!FLAGS_SET(flags, REMOVE_PHYSICAL)) {
                         struct statfs s;
 
@@ -313,26 +373,15 @@ int rm_rf(const char *path, RemoveFlags flags) {
                                                        path);
                 }
 
-                        if (unlink(path) < 0) {
-                                if (FLAGS_SET(flags, REMOVE_MISSING_OK) && errno == ENOENT)
-                                        return 0;
-
-                                return -errno;
-                        }
+                r = 0;
+                q = RET_NERRNO(unlink(path));
         }
 
-                return 0;
-        }
-
-        r = rm_rf_children(fd, flags, NULL);
-
-        if (FLAGS_SET(flags, REMOVE_ROOT) &&
-            rmdir(path) < 0 &&
-            r >= 0 &&
-            (!FLAGS_SET(flags, REMOVE_MISSING_OK) || errno != ENOENT))
-                r = -errno;
-
+        if (r < 0)
                 return r;
+        if (q < 0 && (q != -ENOENT || !FLAGS_SET(flags, REMOVE_MISSING_OK)))
+                return q;
+        return 0;
 }
 
 int rm_rf_child(int fd, const char *name, RemoveFlags flags) {
@@ -351,5 +400,5 @@ int rm_rf_child(int fd, const char *name, RemoveFlags flags) {
         if (FLAGS_SET(flags, REMOVE_ONLY_DIRECTORIES|REMOVE_SUBVOLUME))
                 return -EINVAL;
 
-        return rm_rf_children_inner(fd, name, -1, flags, NULL);
+        return rm_rf_inner_child(fd, name, -1, flags, NULL, true);
 }

src/systemctl/systemctl-logind.c

@@ -297,7 +297,6 @@ int logind_schedule_shutdown(void) {
 #if ENABLE_LOGIND
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         const char *action;
-        const char *log_action;
         sd_bus *bus;
         int r;
 
@@ -308,24 +307,19 @@ int logind_schedule_shutdown(void) {
         switch (arg_action) {
         case ACTION_HALT:
                 action = "halt";
-                log_action = "Shutdown";
                 break;
         case ACTION_POWEROFF:
                 action = "poweroff";
-                log_action = "Shutdown";
                 break;
         case ACTION_KEXEC:
                 action = "kexec";
-                log_action = "Reboot via kexec";
                 break;
         case ACTION_EXIT:
                 action = "exit";
-                log_action = "Shutdown";
                 break;
         case ACTION_REBOOT:
         default:
                 action = "reboot";
-                log_action = "Reboot";
                 break;
         }
 
@@ -339,9 +333,8 @@ int logind_schedule_shutdown(void) {
                 return log_warning_errno(r, "Failed to call ScheduleShutdown in logind, proceeding with immediate shutdown: %s", bus_error_message(&error, r));
 
         if (!arg_quiet)
-                log_info("%s scheduled for %s, use 'shutdown -c' to cancel.",
-                         log_action,
-                         FORMAT_TIMESTAMP_STYLE(arg_when, arg_timestamp_style));
+                logind_show_shutdown();
+
         return 0;
 #else
         return log_error_errno(SYNTHETIC_ERRNO(ENOSYS),
@@ -396,9 +389,16 @@ int logind_show_shutdown(void) {
         if (isempty(action))
                 return log_error_errno(SYNTHETIC_ERRNO(ENODATA), "No scheduled shutdown.");
 
+        if (streq(action, "halt") || streq(action, "poweroff") || streq(action, "exit"))
+                action = "Shutdown";
+        else if (streq(action, "kexec"))
+                action = "Reboot via kexec";
+        else if (streq(action, "reboot"))
+                action = "Reboot";
+
         log_info("%s scheduled for %s, use 'shutdown -c' to cancel.",
                  action,
-                 FORMAT_TIMESTAMP_STYLE(arg_when, arg_timestamp_style));
+                 FORMAT_TIMESTAMP_STYLE(elapse, arg_timestamp_style));
 
         return 0;
 #else

src/xdg-autostart-generator/xdg-autostart-generator.c

@@ -44,38 +44,42 @@ static int enumerate_xdg_autostart(Hashmap *all_services) {
         STRV_FOREACH(path, autostart_dirs) {
                 _cleanup_closedir_ DIR *d = NULL;
 
+                log_debug("Scanning autostart directory \"%s\"…", *path);
                 d = opendir(*path);
                 if (!d) {
-                        if (errno != ENOENT)
-                                log_warning_errno(errno, "Opening %s failed, ignoring: %m", *path);
+                        log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno,
+                                       "Opening %s failed, ignoring: %m", *path);
                         continue;
                 }
 
                 FOREACH_DIRENT(de, d, log_warning_errno(errno, "Failed to enumerate directory %s, ignoring: %m", *path)) {
-                        _cleanup_free_ char *fpath = NULL, *name = NULL;
-                        _cleanup_(xdg_autostart_service_freep) XdgAutostartService *service = NULL;
                         struct stat st;
-
                         if (fstatat(dirfd(d), de->d_name, &st, 0) < 0) {
-                                log_warning_errno(errno, "stat() failed on %s/%s, ignoring: %m", *path, de->d_name);
+                                log_warning_errno(errno, "%s/%s: stat() failed, ignoring: %m", *path, de->d_name);
                                 continue;
                         }
 
-                        if (!S_ISREG(st.st_mode))
+                        if (!S_ISREG(st.st_mode)) {
+                                log_debug("%s/%s: not a regular file, ignoring.", *path, de->d_name);
                                 continue;
+                        }
 
-                        name = xdg_autostart_service_translate_name(de->d_name);
+                        _cleanup_free_ char *name = xdg_autostart_service_translate_name(de->d_name);
                         if (!name)
                                 return log_oom();
 
-                        if (hashmap_contains(all_services, name))
+                        if (hashmap_contains(all_services, name)) {
+                                log_debug("%s/%s: we have already seen \"%s\", ignoring.",
+                                          *path, de->d_name, name);
                                 continue;
+                        }
 
-                        fpath = path_join(*path, de->d_name);
+                        _cleanup_free_ char *fpath = path_join(*path, de->d_name);
                         if (!fpath)
                                 return log_oom();
 
-                        service = xdg_autostart_service_parse_desktop(fpath);
+                        _cleanup_(xdg_autostart_service_freep) XdgAutostartService *service =
+                                xdg_autostart_service_parse_desktop(fpath);
                         if (!service)
                                 return log_oom();
                         service->name = TAKE_PTR(name);

src/xdg-autostart-generator/xdg-autostart-service.c

@@ -470,6 +470,7 @@ int xdg_autostart_format_exec_start(
 }
 
 static int xdg_autostart_generate_desktop_condition(
+                const XdgAutostartService *service,
                 FILE *f,
                 const char *test_binary,
                 const char *condition) {
@@ -483,7 +484,8 @@ static int xdg_autostart_generate_desktop_condition(
                 r = find_executable(test_binary, &gnome_autostart_condition_path);
                 if (r < 0) {
                         log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r,
-                                       "%s not found: %m", test_binary);
+                                       "%s: ExecCondition executable %s not found, unit will not be started automatically: %m",
+                                       service->path, test_binary);
                         fprintf(f, "# ExecCondition using %s skipped due to missing binary.\n", test_binary);
                         return r;
                 }
@@ -492,6 +494,9 @@ static int xdg_autostart_generate_desktop_condition(
                 if (!e_autostart_condition)
                         return log_oom();
 
+                log_debug("%s: ExecCondition converted to %s --condition \"%s\"…",
+                          service->path, gnome_autostart_condition_path, e_autostart_condition);
+
                 fprintf(f,
                          "ExecCondition=%s --condition \"%s\"\n",
                          gnome_autostart_condition_path,
@@ -502,7 +507,7 @@ static int xdg_autostart_generate_desktop_condition(
 }
 
 int xdg_autostart_service_generate_unit(
-                XdgAutostartService *service,
+                const XdgAutostartService *service,
                 const char *dest) {
 
         _cleanup_free_ char *path_escaped = NULL, *exec_start = NULL, *unit = NULL;
@@ -513,23 +518,23 @@ int xdg_autostart_service_generate_unit(
 
         /* Nothing to do for hidden services. */
         if (service->hidden) {
-                log_debug("Not generating service for XDG autostart %s, it is hidden.", service->name);
+                log_debug("%s: not generating unit, entry is hidden.", service->path);
                 return 0;
         }
 
         if (service->systemd_skip) {
-                log_debug("Not generating service for XDG autostart %s, should be skipped by generator.", service->name);
+                log_debug("%s: not generating unit, marked as skipped by generator.", service->path);
                 return 0;
         }
 
         /* Nothing to do if type is not Application. */
         if (!streq_ptr(service->type, "Application")) {
-                log_debug("Not generating service for XDG autostart %s, only Type=Application is supported.", service->name);
+                log_debug("%s: not generating unit, Type=%s is not supported.", service->path, service->type);
                 return 0;
         }
 
         if (!service->exec_string) {
-                log_warning("Not generating service for XDG autostart %s, it is has no Exec= line.", service->name);
+                log_warning("%s: not generating unit, no Exec= line.", service->path);
                 return 0;
         }
 
@@ -539,24 +544,21 @@ int xdg_autostart_service_generate_unit(
                 r = find_executable(service->try_exec, NULL);
                 if (r < 0) {
                         log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_WARNING, r,
-                                       "Not generating service for XDG autostart %s, could not find TryExec= binary %s: %m",
-                                       service->name, service->try_exec);
+                                       "%s: not generating unit, could not find TryExec= binary %s: %m",
+                                       service->path, service->try_exec);
                         return 0;
                 }
         }
 
         r = xdg_autostart_format_exec_start(service->exec_string, &exec_start);
         if (r < 0) {
-                log_warning_errno(r,
-                                  "Not generating service for XDG autostart %s, error parsing Exec= line: %m",
-                                  service->name);
+                log_warning_errno(r, "%s: not generating unit, error parsing Exec= line: %m", service->path);
                 return 0;
         }
 
         if (service->gnome_autostart_phase) {
                 /* There is no explicit value for the "Application" phase. */
-                log_debug("Not generating service for XDG autostart %s, startup phases are not supported.",
-                          service->name);
+                log_debug("%s: not generating unit, startup phases are not supported.", service->path);
                 return 0;
         }
 
@@ -570,7 +572,7 @@ int xdg_autostart_service_generate_unit(
 
         f = fopen(unit, "wxe");
         if (!f)
-                return log_error_errno(errno, "Failed to create unit file %s: %m", unit);
+                return log_error_errno(errno, "%s: failed to create unit file %s: %m", service->path, unit);
 
         fprintf(f,
                 "# Automatically generated by systemd-xdg-autostart-generator\n\n"
@@ -635,19 +637,18 @@ int xdg_autostart_service_generate_unit(
                         e_not_show_in);
         }
 
-        r = xdg_autostart_generate_desktop_condition(f,
+        r = xdg_autostart_generate_desktop_condition(service, f,
                                                      "gnome-systemd-autostart-condition",
                                                      service->autostart_condition);
         if (r < 0)
                 return r;
 
-        r = xdg_autostart_generate_desktop_condition(f,
+        r = xdg_autostart_generate_desktop_condition(service, f,
                                                      "kde-systemd-start-condition",
                                                      service->kde_autostart_condition);
         if (r < 0)
                 return r;
 
-        (void) generator_add_symlink(dest, "xdg-desktop-autostart.target", "wants", service->name);
-
-        return 0;
+        log_debug("%s: symlinking %s in xdg-desktop-autostart.target/.wants…", service->path, service->name);
+        return generator_add_symlink(dest, "xdg-desktop-autostart.target", "wants", service->name);
 }

src/xdg-autostart-generator/xdg-autostart-service.h

@@ -26,7 +26,6 @@ typedef struct XdgAutostartService {
 
 } XdgAutostartService;
 
-
 XdgAutostartService * xdg_autostart_service_free(XdgAutostartService *s);
 DEFINE_TRIVIAL_CLEANUP_FUNC(XdgAutostartService*, xdg_autostart_service_free);
 
@@ -34,4 +33,4 @@ char *xdg_autostart_service_translate_name(const char *name);
 int xdg_autostart_format_exec_start(const char *exec, char **ret_exec_start);
 
 XdgAutostartService *xdg_autostart_service_parse_desktop(const char *path);
-int xdg_autostart_service_generate_unit(XdgAutostartService *service, const char *dest);
+int xdg_autostart_service_generate_unit(const XdgAutostartService *service, const char *dest);