From ee0e6e54479f8ad1991e531bb1e931b696d67aaf Mon Sep 17 00:00:00 2001

From: Lennart Poettering <lennart@poettering.net>

Date: Mon, 2 Nov 2015 23:14:30 +0100

Subject: [PATCH] sd-journal: various clean-ups and modernizations

- Always print a debug log message about files and directories we cannot

  open right when it happens instead of the caller, thus reducing the

  number of places where we need to generate the debug message.

- Always push the errors we encounter immediately into the error set,

  when we run into them, instead of in the caller. Thus, we never forget

  to push them in.

- Use stack instead of heap memory where we can.

- Make remove_file() void, since it cannot fail anyway and always

  returned 0.

- Make local machine check of journal directories explicit in a

  function, to make things more readable.

- Port to all directory listing loops FOREACH_DIRENT_ALL()

- sd-daemon is library code, hence never log at higher log levels than

  LOG_DEBUG.

(cherry picked from commit d617408ecbe69db69aefddfcb10a6c054ea46ba0)

Related: #1465759

---

 src/journal/sd-journal.c | 242 ++++++++++++++++++++++-------------------------

 1 file changed, 111 insertions(+), 131 deletions(-)

diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c

index 3749f9e89..9895d9608 100644

--- a/src/journal/sd-journal.c

+++ b/src/journal/sd-journal.c

@@ -1171,6 +1171,8 @@ static bool file_has_type_prefix(const char *prefix, const char *filename) {

 }

 static bool file_type_wanted(int flags, const char *filename) {

+        assert(filename);

+

         if (!endswith(filename, ".journal") && !endswith(filename, ".journal~"))

                 return false;

@@ -1195,7 +1197,7 @@ static bool file_type_wanted(int flags, const char *filename) {

 static int add_any_file(sd_journal *j, const char *path) {

         JournalFile *f = NULL;

-        int r;

+        int r, k;

         assert(j);

         assert(path);

@@ -1204,20 +1206,23 @@ static int add_any_file(sd_journal *j, const char *path) {

                 return 0;

         if (ordered_hashmap_size(j->files) >= JOURNAL_FILES_MAX) {

-                log_warning("Too many open journal files, not adding %s.", path);

-                return set_put_error(j, -ETOOMANYREFS);

+                log_debug("Too many open journal files, not adding %s.", path);

+                r = -ETOOMANYREFS;

+                goto fail;

         }

         r = journal_file_open(path, O_RDONLY, 0, false, false, NULL, j->mmap, NULL, &f);

-        if (r < 0)

-                return r;

+        if (r < 0) {

+                log_debug_errno(r, "Failed to open journal file %s: %m", path);

+                goto fail;

+        }

         /* journal_file_dump(f); */

         r = ordered_hashmap_put(j->files, f->path, f);

         if (r < 0) {

                 journal_file_close(f);

-                return r;

+                goto fail;

         }

         log_debug("File %s added.", f->path);

@@ -1227,10 +1232,17 @@ static int add_any_file(sd_journal *j, const char *path) {

         j->current_invalidate_counter ++;

         return 0;

+

+fail:

+        k = set_put_error(j, r);

+        if (k < 0)

+                return k;

+

+        return r;

 }

 static int add_file(sd_journal *j, const char *prefix, const char *filename) {

-        char *path = NULL;

+        const char *path;

         assert(j);

         assert(prefix);

@@ -1250,24 +1262,20 @@ static int add_file(sd_journal *j, const char *prefix, const char *filename) {

         return add_any_file(j, path);

 }

-static int remove_file(sd_journal *j, const char *prefix, const char *filename) {

-        _cleanup_free_ char *path;

+static void remove_file(sd_journal *j, const char *prefix, const char *filename) {

+        const char *path;

         JournalFile *f;

         assert(j);

         assert(prefix);

         assert(filename);

-        path = strjoin(prefix, "/", filename, NULL);

-        if (!path)

-                return -ENOMEM;

-

+        path = strjoina(prefix, "/", filename);

         f = ordered_hashmap_get(j->files, path);

         if (!f)

-                return 0;

+                return;

         remove_file_real(j, f);

-        return 0;

 }

 static void remove_file_real(sd_journal *j, JournalFile *f) {

@@ -1296,12 +1304,27 @@ static void remove_file_real(sd_journal *j, JournalFile *f) {

         j->current_invalidate_counter ++;

 }

+static int dirname_is_machine_id(const char *fn) {

+        sd_id128_t id, machine;

+        int r;

+

+        r = sd_id128_get_machine(&machine);

+        if (r < 0)

+                return r;

+

+        r = sd_id128_from_string(fn, &id;;

+        if (r < 0)

+                return r;

+

+        return sd_id128_equal(id, machine);

+}

+

 static int add_directory(sd_journal *j, const char *prefix, const char *dirname) {

         _cleanup_free_ char *path = NULL;

-        int r;

         _cleanup_closedir_ DIR *d = NULL;

-        sd_id128_t id, mid;

+        struct dirent *de = NULL;

         Directory *m;

+        int r, k;

         assert(j);

         assert(prefix);

@@ -1310,35 +1333,36 @@ static int add_directory(sd_journal *j, const char *prefix, const char *dirname)

         log_debug("Considering %s/%s.", prefix, dirname);

         if ((j->flags & SD_JOURNAL_LOCAL_ONLY) &&

-            (sd_id128_from_string(dirname, &id) < 0 ||

-             sd_id128_get_machine(&mid) < 0 ||

-             !(sd_id128_equal(id, mid) || path_startswith(prefix, "/run"))))

+            !(dirname_is_machine_id(dirname) > 0 || path_startswith(prefix, "/run")))

             return 0;

         path = strjoin(prefix, "/", dirname, NULL);

-        if (!path)

-                return -ENOMEM;

+        if (!path) {

+                r = -ENOMEM;

+                goto fail;

+        }

         d = opendir(path);

         if (!d) {

-                log_debug_errno(errno, "Failed to open %s: %m", path);

-                if (errno == ENOENT)

-                        return 0;

-                return -errno;

+                r = log_debug_errno(errno, "Failed to open directory %s: %m", path);

+                goto fail;

         }

         m = hashmap_get(j->directories_by_path, path);

         if (!m) {

                 m = new0(Directory, 1);

-                if (!m)

-                        return -ENOMEM;

+                if (!m) {

+                        r = -ENOMEM;

+                        goto fail;

+                }

                 m->is_root = false;

                 m->path = path;

                 if (hashmap_put(j->directories_by_path, m->path, m) < 0) {

                         free(m);

-                        return -ENOMEM;

+                        r = -ENOMEM;

+                        goto fail;

                 }

                 path = NULL; /* avoid freeing in cleanup */

@@ -1360,41 +1384,30 @@ static int add_directory(sd_journal *j, const char *prefix, const char *dirname)

                         inotify_rm_watch(j->inotify_fd, m->wd);

         }

-        for (;;) {

-                struct dirent *de;

-

-                errno = 0;

-                de = readdir(d);

-                if (!de && errno != 0) {

-                        r = -errno;

-                        log_debug_errno(errno, "Failed to read directory %s: %m", m->path);

-                        return r;

-                }

-                if (!de)

-                        break;

+        FOREACH_DIRENT_ALL(de, d, return log_debug_errno(errno, "Failed to read directory %s: %m", m->path)) {

                 if (dirent_is_file_with_suffix(de, ".journal") ||

-                    dirent_is_file_with_suffix(de, ".journal~")) {

-                        r = add_file(j, m->path, de->d_name);

-                        if (r < 0) {

-                                log_debug_errno(r, "Failed to add file %s/%s: %m",

-                                                m->path, de->d_name);

-                                r = set_put_error(j, r);

-                                if (r < 0)

-                                        return r;

-                        }

-                }

+                    dirent_is_file_with_suffix(de, ".journal~"))

+                        (void) add_file(j, m->path, de->d_name);

         }

         check_network(j, dirfd(d));

         return 0;

+

+fail:

+        k = set_put_error(j, r);

+        if (k < 0)

+                return k;

+

+        return r;

 }

-static int add_root_directory(sd_journal *j, const char *p) {

+static int add_root_directory(sd_journal *j, const char *p, bool missing_ok) {

         _cleanup_closedir_ DIR *d = NULL;

+        struct dirent *de;

         Directory *m;

-        int r;

+        int r, k;

         assert(j);

         assert(p);

@@ -1407,26 +1420,35 @@ static int add_root_directory(sd_journal *j, const char *p) {

                 p = strjoina(j->prefix, p);

         d = opendir(p);

-        if (!d)

-                return -errno;

+        if (!d) {

+                if (errno == ENOENT && missing_ok)

+                        return 0;

+

+                r = log_debug_errno(errno, "Failed to open root directory %s: %m", p);

+                goto fail;

+        }

         m = hashmap_get(j->directories_by_path, p);

         if (!m) {

                 m = new0(Directory, 1);

-                if (!m)

-                        return -ENOMEM;

+                if (!m) {

+                        r = -ENOMEM;

+                        goto fail;

+                }

                 m->is_root = true;

                 m->path = strdup(p);

                 if (!m->path) {

                         free(m);

-                        return -ENOMEM;

+                        r = -ENOMEM;

+                        goto fail;

                 }

                 if (hashmap_put(j->directories_by_path, m->path, m) < 0) {

                         free(m->path);

                         free(m);

-                        return -ENOMEM;

+                        r = -ENOMEM;

+                        goto fail;

                 }

                 j->current_invalidate_counter ++;

@@ -1449,42 +1471,27 @@ static int add_root_directory(sd_journal *j, const char *p) {

         if (j->no_new_files)

                 return 0;

-        for (;;) {

-                struct dirent *de;

+        FOREACH_DIRENT_ALL(de, d, return log_debug_errno(errno, "Failed to read directory %s: %m", m->path)) {

                 sd_id128_t id;

-                errno = 0;

-                de = readdir(d);

-                if (!de && errno != 0) {

-                        r = -errno;

-                        log_debug_errno(errno, "Failed to read directory %s: %m", m->path);

-                        return r;

-                }

-                if (!de)

-                        break;

-

                 if (dirent_is_file_with_suffix(de, ".journal") ||

-                    dirent_is_file_with_suffix(de, ".journal~")) {

-                        r = add_file(j, m->path, de->d_name);

-                        if (r < 0) {

-                                log_debug_errno(r, "Failed to add file %s/%s: %m",

-                                                m->path, de->d_name);

-                                r = set_put_error(j, r);

-                                if (r < 0)

-                                        return r;

-                        }

-                } else if ((de->d_type == DT_DIR || de->d_type == DT_LNK || de->d_type == DT_UNKNOWN) &&

-                           sd_id128_from_string(de->d_name, &id) >= 0) {

-

-                        r = add_directory(j, m->path, de->d_name);

-                        if (r < 0)

-                                log_debug_errno(r, "Failed to add directory %s/%s: %m", m->path, de->d_name);

-                }

+                    dirent_is_file_with_suffix(de, ".journal~"))

+                        (void) add_file(j, m->path, de->d_name);

+                else if (IN_SET(de->d_type, DT_DIR, DT_LNK, DT_UNKNOWN) &&

+                         sd_id128_from_string(de->d_name, &id) >= 0)

+                        (void) add_directory(j, m->path, de->d_name);

         }

         check_network(j, dirfd(d));

         return 0;

+

+fail:

+        k = set_put_error(j, r);

+        if (k < 0)

+                return k;

+

+        return r;

 }

 static void remove_directory(sd_journal *j, Directory *d) {

@@ -1509,8 +1516,8 @@ static void remove_directory(sd_journal *j, Directory *d) {

 }

 static int add_search_paths(sd_journal *j) {

-        int r;

-        const char search_paths[] =

+

+        static const char search_paths[] =

                 "/run/log/journal\0"

                 "/var/log/journal\0";

         const char *p;

@@ -1520,14 +1527,8 @@ static int add_search_paths(sd_journal *j) {

         /* We ignore most errors here, since the idea is to only open

          * what's actually accessible, and ignore the rest. */

-        NULSTR_FOREACH(p, search_paths) {

-                r = add_root_directory(j, p);

-                if (r < 0 && r != -ENOENT) {

-                        r = set_put_error(j, r);

-                        if (r < 0)

-                                return r;

-                }

-        }

+        NULSTR_FOREACH(p, search_paths)

+                (void) add_root_directory(j, p, true);

         return 0;

 }

@@ -1551,17 +1552,14 @@ static int add_current_paths(sd_journal *j) {

                 if (!dir)

                         return -ENOMEM;

-                r = add_root_directory(j, dir);

-                if (r < 0) {

-                        set_put_error(j, r);

+                r = add_root_directory(j, dir, true);

+                if (r < 0)

                         return r;

-                }

         }

         return 0;

 }

-

 static int allocate_inotify(sd_journal *j) {

         assert(j);

@@ -1689,11 +1687,9 @@ _public_ int sd_journal_open_directory(sd_journal **ret, const char *path, int f

         if (!j)

                 return -ENOMEM;

-        r = add_root_directory(j, path);

-        if (r < 0) {

-                set_put_error(j, r);

+        r = add_root_directory(j, path, false);

+        if (r < 0)

                 goto fail;

-        }

         *ret = j;

         return 0;

@@ -1718,10 +1714,8 @@ _public_ int sd_journal_open_files(sd_journal **ret, const char **paths, int fla

         STRV_FOREACH(path, paths) {

                 r = add_any_file(j, *path);

-                if (r < 0) {

-                        log_error_errno(r, "Failed to open %s: %m", *path);

+                if (r < 0)

                         goto fail;

-                }

         }

         j->no_new_files = true;

@@ -2061,7 +2055,7 @@ _public_ int sd_journal_get_fd(sd_journal *j) {

         if (j->no_new_files)

                 r = add_current_paths(j);

         else if (j->path)

-                r = add_root_directory(j, j->path);

+                r = add_root_directory(j, j->path, true);

         else

                 r = add_search_paths(j);

         if (r < 0)

@@ -2108,7 +2102,6 @@ _public_ int sd_journal_get_timeout(sd_journal *j, uint64_t *timeout_usec) {

 static void process_inotify_event(sd_journal *j, struct inotify_event *e) {

         Directory *d;

-        int r;

         assert(j);

         assert(e);

@@ -2124,20 +2117,10 @@ static void process_inotify_event(sd_journal *j, struct inotify_event *e) {

                         /* Event for a journal file */

-                        if (e->mask & (IN_CREATE|IN_MOVED_TO|IN_MODIFY|IN_ATTRIB)) {

-                                r = add_file(j, d->path, e->name);

-                                if (r < 0) {

-                                        log_debug_errno(r, "Failed to add file %s/%s: %m",

-                                                        d->path, e->name);

-                                        set_put_error(j, r);

-                                }

-

-                        } else if (e->mask & (IN_DELETE|IN_MOVED_FROM|IN_UNMOUNT)) {

-

-                                r = remove_file(j, d->path, e->name);

-                                if (r < 0)

-                                        log_debug_errno(r, "Failed to remove file %s/%s: %m", d->path, e->name);

-                        }

+                        if (e->mask & (IN_CREATE|IN_MOVED_TO|IN_MODIFY|IN_ATTRIB))

+                                (void) add_file(j, d->path, e->name);

+                        else if (e->mask & (IN_DELETE|IN_MOVED_FROM|IN_UNMOUNT))

+                                remove_file(j, d->path, e->name);

                 } else if (!d->is_root && e->len == 0) {

@@ -2150,11 +2133,8 @@ static void process_inotify_event(sd_journal *j, struct inotify_event *e) {

                         /* Event for root directory */

-                        if (e->mask & (IN_CREATE|IN_MOVED_TO|IN_MODIFY|IN_ATTRIB)) {

-                                r = add_directory(j, d->path, e->name);

-                                if (r < 0)

-                                        log_debug_errno(r, "Failed to add directory %s/%s: %m", d->path, e->name);

-                        }

+                        if (e->mask & (IN_CREATE|IN_MOVED_TO|IN_MODIFY|IN_ATTRIB))

+                                (void) add_directory(j, d->path, e->name);

                 }

                 return;

@@ -2163,7 +2143,7 @@ static void process_inotify_event(sd_journal *j, struct inotify_event *e) {

         if (e->mask & IN_IGNORED)

                 return;

-        log_warning("Unknown inotify event.");

+        log_debug("Unknown inotify event.");

 }

 static int determine_change(sd_journal *j) {