From d617408ecbe69db69aefddfcb10a6c054ea46ba0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:14:30 +0100 Subject: 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. --- src/journal/sd-journal.c | 253 +++++++++++++++++++++-------------------------- 1 file changed, 113 insertions(+), 140 deletions(-) (limited to 'src/journal/sd-journal.c') diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c index 218df8cebe..b5e1d05fa6 100644 --- a/src/journal/sd-journal.c +++ b/src/journal/sd-journal.c @@ -1183,6 +1183,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; @@ -1207,7 +1209,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); @@ -1216,20 +1218,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); @@ -1239,11 +1244,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) { - _cleanup_free_ char *path = NULL; - int r; + const char *path; assert(j); assert(prefix); @@ -1253,34 +1264,24 @@ static int add_file(sd_journal *j, const char *prefix, const char *filename) { !file_type_wanted(j->flags, filename)) return 0; - path = strjoin(prefix, "/", filename, NULL); - if (!path) - return -ENOMEM; - - r = add_any_file(j, path); - if (r == -ENOENT) - return 0; - return r; + path = strjoina(prefix, "/", 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) { @@ -1309,12 +1310,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); @@ -1323,35 +1339,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 */ @@ -1373,41 +1390,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); @@ -1420,26 +1426,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 ++; @@ -1462,42 +1477,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) { @@ -1522,8 +1522,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; @@ -1533,14 +1533,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; } @@ -1564,17 +1558,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); @@ -1702,11 +1693,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; @@ -1731,10 +1720,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; @@ -2074,7 +2061,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) @@ -2121,7 +2108,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); @@ -2137,20 +2123,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) { @@ -2163,11 +2139,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; @@ -2176,7 +2149,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) { -- cgit v1.2.3-54-g00ecf From 5768d2594940668506bb4cafa078f654cc20dc5a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:37:05 +0100 Subject: journalctl: when we fail to open a journal file, print why When we enumerate journal files and encounter an invalid one, remember which this, and show it to the user. Note the possibly slightly surprising logic here: we store only one path per error code. This means we show all error kinds but not every actual error we encounter. This has the benefit of not requiring us to keep a potentially unbounded list of errors with their sources around, but can still provide a pretty complete overview on the errors we encountered. Fixes #1669. --- src/journal/journal-internal.h | 2 +- src/journal/journalctl.c | 27 ++++++++++++++++++----- src/journal/sd-journal.c | 49 ++++++++++++++++++++++++++++++++++-------- 3 files changed, 63 insertions(+), 15 deletions(-) (limited to 'src/journal/sd-journal.c') diff --git a/src/journal/journal-internal.h b/src/journal/journal-internal.h index 1221799c1d..06847402e0 100644 --- a/src/journal/journal-internal.h +++ b/src/journal/journal-internal.h @@ -121,7 +121,7 @@ struct sd_journal { Hashmap *directories_by_path; Hashmap *directories_by_wd; - Set *errors; + Hashmap *errors; }; char *journal_make_match_string(sd_journal *j); diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index 30ae6e39c3..98a852cb50 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -1708,33 +1708,50 @@ static int access_check_var_log_journal(sd_journal *j) { static int access_check(sd_journal *j) { Iterator it; void *code; + char *path; int r = 0; assert(j); - if (set_isempty(j->errors)) { + if (hashmap_isempty(j->errors)) { if (ordered_hashmap_isempty(j->files)) log_notice("No journal files were found."); return 0; } - if (set_contains(j->errors, INT_TO_PTR(-EACCES))) { + if (hashmap_contains(j->errors, INT_TO_PTR(-EACCES))) { (void) access_check_var_log_journal(j); if (ordered_hashmap_isempty(j->files)) r = log_error_errno(EACCES, "No journal files were opened due to insufficient permissions."); } - SET_FOREACH(code, j->errors, it) { + HASHMAP_FOREACH_KEY(path, code, j->errors, it) { int err; err = abs(PTR_TO_INT(code)); - if (err == EACCES) + switch (err) { + case EACCES: continue; - log_warning_errno(err, "An error was encountered while opening journal files, ignoring: %m"); + case ENODATA: + log_warning_errno(err, "Journal file %s is truncated, ignoring file.", path); + break; + + case EPROTONOSUPPORT: + log_warning_errno(err, "Journal file %s uses an unsupported feature, ignoring file.", path); + break; + + case EBADMSG: + log_warning_errno(err, "Journal file %s corrupted, ignoring file.", path); + break; + + default: + log_warning_errno(err, "An error was encountered while opening journal file %s, ignoring file.", path); + break; + } } return r; diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c index b5e1d05fa6..5cde7f17f7 100644 --- a/src/journal/sd-journal.c +++ b/src/journal/sd-journal.c @@ -73,19 +73,46 @@ static bool journal_pid_changed(sd_journal *j) { return j->original_pid != getpid(); } -/* We return an error here only if we didn't manage to - memorize the real error. */ -static int set_put_error(sd_journal *j, int r) { +static int journal_put_error(sd_journal *j, int r, const char *path) { + char *copy; int k; + /* Memorize an error we encountered, and store which + * file/directory it was generated from. Note that we store + * only *one* path per error code, as the error code is the + * key into the hashmap, and the path is the value. This means + * we keep track only of all error kinds, but not of all error + * locations. This has the benefit that the hashmap cannot + * grow beyond bounds. + * + * We return an error here only if we didn't manage to + * memorize the real error. */ + if (r >= 0) return r; - k = set_ensure_allocated(&j->errors, NULL); + k = hashmap_ensure_allocated(&j->errors, NULL); if (k < 0) return k; - return set_put(j->errors, INT_TO_PTR(r)); + if (path) { + copy = strdup(path); + if (!copy) + return -ENOMEM; + } else + copy = NULL; + + k = hashmap_put(j->errors, INT_TO_PTR(r), copy); + if (k < 0) { + free(copy); + + if (k == -EEXIST) + return 0; + + return k; + } + + return 0; } static void detach_location(sd_journal *j) { @@ -1246,7 +1273,7 @@ static int add_any_file(sd_journal *j, const char *path) { return 0; fail: - k = set_put_error(j, r); + k = journal_put_error(j, r, path); if (k < 0) return k; @@ -1402,7 +1429,7 @@ static int add_directory(sd_journal *j, const char *prefix, const char *dirname) return 0; fail: - k = set_put_error(j, r); + k = journal_put_error(j, r, path ?: dirname); if (k < 0) return k; @@ -1493,7 +1520,7 @@ static int add_root_directory(sd_journal *j, const char *p, bool missing_ok) { return 0; fail: - k = set_put_error(j, r); + k = journal_put_error(j, r, p); if (k < 0) return k; @@ -1738,6 +1765,7 @@ fail: _public_ void sd_journal_close(sd_journal *j) { Directory *d; JournalFile *f; + char *p; if (!j) return; @@ -1765,10 +1793,13 @@ _public_ void sd_journal_close(sd_journal *j) { mmap_cache_unref(j->mmap); } + while ((p = hashmap_steal_first(j->errors))) + free(p); + hashmap_free(j->errors); + free(j->path); free(j->prefix); free(j->unique_field); - set_free(j->errors); free(j); } -- cgit v1.2.3-54-g00ecf