From 9fe4ea21bec739bfe0ebac5565f0539b0e25b317 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 1 Nov 2015 20:28:55 +0100 Subject: string-util: rework memory_erase() so that it cannot be optimized away memory_erase() so far just called memset(), which the compiler might optimize away under certain conditions if it feels there's benefit in it. C11 knows a new memset_s() call that is like memset(), but may not be optimized away. Ideally, we'd just use that call, but glibc currently does not support it. Hence, implement our own simplistic version of it. We use a GCC pragma to turn off optimization for this call, and also use the "volatile" keyword on the pointers to ensure that gcc will use the pointers as-is. According to a variety of internet sources, either one does the trick. However, there are also reports that at least the volatile thing isn't fully correct, hence let's add some snake oil and employ both techniques. https://news.ycombinator.com/item?id=4711346 --- .gitignore | 1 + Makefile.am | 7 ++++++ src/basic/string-util.c | 31 +++++++++++++++++------ src/basic/string-util.h | 4 +-- src/test/test-string-util.c | 61 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 src/test/test-string-util.c diff --git a/.gitignore b/.gitignore index 7abe8e7dae..1c006b53f8 100644 --- a/.gitignore +++ b/.gitignore @@ -256,6 +256,7 @@ /test-socket-util /test-ssd /test-strbuf +/test-string-util /test-strip-tab-ansi /test-strv /test-strxcpyx diff --git a/Makefile.am b/Makefile.am index be001bd09e..a601265d78 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1447,6 +1447,7 @@ tests += \ test-utf8 \ test-ellipsize \ test-util \ + test-string-util \ test-extract-word \ test-parse-util \ test-user-util \ @@ -1748,6 +1749,12 @@ test_util_SOURCES = \ test_util_LDADD = \ libshared.la +test_string_util_SOURCES = \ + src/test/test-string-util.c + +test_string_util_LDADD = \ + libshared.la + test_extract_word_SOURCES = \ src/test/test-extract-word.c diff --git a/src/basic/string-util.c b/src/basic/string-util.c index 63b9b79df9..c3be576816 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -748,23 +748,38 @@ int free_and_strdup(char **p, const char *s) { return 1; } -void string_erase(char *x) { +#pragma GCC push_options +#pragma GCC optimize("O0") + +void* memory_erase(void *p, size_t l) { + volatile uint8_t* x = (volatile uint8_t*) p; + + /* This basically does what memset() does, but hopefully isn't + * optimized away by the compiler. One of those days, when + * glibc learns memset_s() we should replace this call by + * memset_s(), but until then this has to do. */ + + for (; l > 0; l--) + *(x++) = 'x'; + + return p; +} + +#pragma GCC pop_options + +char* string_erase(char *x) { if (!x) - return; + return NULL; /* A delicious drop of snake-oil! To be called on memory where * we stored passphrases or so, after we used them. */ - memory_erase(x, strlen(x)); + return memory_erase(x, strlen(x)); } char *string_free_erase(char *s) { - if (!s) - return NULL; - - string_erase(s); - return mfree(s); + return mfree(string_erase(s)); } bool string_is_safe(const char *p) { diff --git a/src/basic/string-util.h b/src/basic/string-util.h index 297b8f8232..15244b8184 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -162,8 +162,8 @@ static inline void *memmem_safe(const void *haystack, size_t haystacklen, const return memmem(haystack, haystacklen, needle, needlelen); } -#define memory_erase(p, l) memset((p), 'x', (l)) -void string_erase(char *x); +void* memory_erase(void *p, size_t l); +char *string_erase(char *x); char *string_free_erase(char *s); DEFINE_TRIVIAL_CLEANUP_FUNC(char *, string_free_erase); diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c new file mode 100644 index 0000000000..25444c794a --- /dev/null +++ b/src/test/test-string-util.c @@ -0,0 +1,61 @@ +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/ + +/*** + This file is part of systemd. + + Copyright 2015 Lennart Poettering + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include "string-util.h" + +static void test_string_erase(void) { + char *x; + + x = strdupa(""); + assert_se(streq(string_erase(x), "")); + + x = strdupa("1"); + assert_se(streq(string_erase(x), "x")); + + x = strdupa("12"); + assert_se(streq(string_erase(x), "xx")); + + x = strdupa("123"); + assert_se(streq(string_erase(x), "xxx")); + + x = strdupa("1234"); + assert_se(streq(string_erase(x), "xxxx")); + + x = strdupa("12345"); + assert_se(streq(string_erase(x), "xxxxx")); + + x = strdupa("123456"); + assert_se(streq(string_erase(x), "xxxxxx")); + + x = strdupa("1234567"); + assert_se(streq(string_erase(x), "xxxxxxx")); + + x = strdupa("12345678"); + assert_se(streq(string_erase(x), "xxxxxxxx")); + + x = strdupa("123456789"); + assert_se(streq(string_erase(x), "xxxxxxxxx")); +} + +int main(int argc, char *argv[]) { + test_string_erase(); + return 0; +} -- cgit v1.2.3-54-g00ecf From 19854865a877a3a4fa3d04550c15a99c0e1187ff Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 09:34:05 +0100 Subject: core: bump net.unix.max_dgram_qlen really early during boot Only that way it actually has an effect on all our sockets, including $NOTIFY_SOCKET. --- src/basic/def.h | 3 +++ src/core/main.c | 41 ++++++++++++++++++++++++++++++++++++++++- sysctl.d/50-default.conf | 3 --- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/basic/def.h b/src/basic/def.h index 7c4161eb72..cbef137410 100644 --- a/src/basic/def.h +++ b/src/basic/def.h @@ -35,6 +35,9 @@ * the watchdog pings will keep the loop busy. */ #define DEFAULT_EXIT_USEC (30*USEC_PER_SEC) +/* The default value for the net.unix.max_dgram_qlen sysctl */ +#define DEFAULT_UNIX_MAX_DGRAM_QLEN 512UL + #define SYSTEMD_CGROUP_CONTROLLER "name=systemd" #define SIGNALS_CRASH_HANDLER SIGSEGV,SIGILL,SIGFPE,SIGBUS,SIGQUIT,SIGABRT diff --git a/src/core/main.c b/src/core/main.c index 593b974566..dd21b26971 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1242,12 +1242,50 @@ static int status_welcome(void) { static int write_container_id(void) { const char *c; + int r; c = getenv("container"); if (isempty(c)) return 0; - return write_string_file("/run/systemd/container", c, WRITE_STRING_FILE_CREATE); + r = write_string_file("/run/systemd/container", c, WRITE_STRING_FILE_CREATE); + if (r < 0) + return log_warning_errno(r, "Failed to write /run/systed/container, ignoring: %m"); + + return 1; +} + +static int bump_unix_max_dgram_qlen(void) { + _cleanup_free_ char *qlen = NULL; + unsigned long v; + int r; + + /* Let's bump the net.unix.max_dgram_qlen sysctl. The kernel + * default of 16 is simply too low. We set the value really + * really early during boot, so that it is actually applied to + * all our sockets, including the $NOTIFY_SOCKET one. */ + + r = read_one_line_file("/proc/sys/net/unix/max_dgram_qlen", &qlen); + if (r < 0) + return log_warning_errno(r, "Failed to read AF_UNIX datagram queue length, ignoring: %m"); + + r = safe_atolu(qlen, &v); + if (r < 0) + return log_warning_errno(r, "Failed to parse AF_UNIX datagram queue length, ignoring: %m"); + + if (v >= DEFAULT_UNIX_MAX_DGRAM_QLEN) + return 0; + + qlen = mfree(qlen); + if (asprintf(&qlen, "%lu\n", DEFAULT_UNIX_MAX_DGRAM_QLEN) < 0) + return log_oom(); + + r = write_string_file("/proc/sys/net/unix/max_dgram_qlen", qlen, 0); + if (r < 0) + return log_full_errno(IN_SET(r, -EROFS, -EPERM, -EACCES) ? LOG_DEBUG : LOG_WARNING, r, + "Failed to bump AF_UNIX datagram queue length, ignoring: %m"); + + return 1; } int main(int argc, char *argv[]) { @@ -1613,6 +1651,7 @@ int main(int argc, char *argv[]) { hostname_setup(); machine_id_setup(NULL); loopback_setup(); + bump_unix_max_dgram_qlen(); test_mtab(); test_usr(); diff --git a/sysctl.d/50-default.conf b/sysctl.d/50-default.conf index 2097551c33..def151bb84 100644 --- a/sysctl.d/50-default.conf +++ b/sysctl.d/50-default.conf @@ -35,9 +35,6 @@ net.ipv4.conf.all.promote_secondaries = 1 # Fair Queue CoDel packet scheduler to fight bufferbloat net.core.default_qdisc = fq_codel -# Make sure we can queue more than just a few datagrams in AF_UNIX sockets. -net.unix.max_dgram_qlen = 512 - # Enable hard and soft link protection fs.protected_hardlinks = 1 fs.protected_symlinks = 1 -- cgit v1.2.3-54-g00ecf From 934ae16baf543af03f3f521277d14524ca772d17 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:07:58 +0100 Subject: proc-cmdline: return proper errors from shall_restore_state() Let's not eat up errors in shall_restore_state(), but in the consumers instead, just for the sake of keeping the library calls generic. --- src/backlight/backlight.c | 2 +- src/basic/proc-cmdline.c | 2 +- src/rfkill/rfkill.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index 08980fba82..b0fa079fec 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -381,7 +381,7 @@ int main(int argc, char *argv[]) { _cleanup_free_ char *value = NULL; const char *clamp; - if (!shall_restore_state()) + if (shall_restore_state() == 0) return EXIT_SUCCESS; if (!validate_device(udev, device)) diff --git a/src/basic/proc-cmdline.c b/src/basic/proc-cmdline.c index dd91ce7dbc..778c994501 100644 --- a/src/basic/proc-cmdline.c +++ b/src/basic/proc-cmdline.c @@ -141,5 +141,5 @@ int shall_restore_state(void) { if (r == 0) return true; - return parse_boolean(value) != 0; + return parse_boolean(value); } diff --git a/src/rfkill/rfkill.c b/src/rfkill/rfkill.c index bb00f99ecf..5c45a3ae6c 100644 --- a/src/rfkill/rfkill.c +++ b/src/rfkill/rfkill.c @@ -212,7 +212,7 @@ static int load_state( assert(udev); assert(event); - if (!shall_restore_state()) + if (shall_restore_state() == 0) return 0; r = find_device(udev, event, &device); -- cgit v1.2.3-54-g00ecf From d8e023aa07029b672fba425e100330ef6343e22d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:09:06 +0100 Subject: core: expose number of file descriptors in fd store on the bus For each service expose how many file descriptors there are currently in the fd store. (Also, fix the exporting of the fdstore limit, given that the field is just an "unsigned" but we exported it as "uint32_t". Not that there way any effective difference, but let's make this clean...) --- src/core/dbus-service.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index c41b3e1723..24f611a593 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -63,7 +63,8 @@ const sd_bus_vtable bus_service_vtable[] = { SD_BUS_PROPERTY("MainPID", "u", bus_property_get_pid, offsetof(Service, main_pid), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("ControlPID", "u", bus_property_get_pid, offsetof(Service, control_pid), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("BusName", "s", NULL, offsetof(Service, bus_name), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("FileDescriptorStoreMax", "u", NULL, offsetof(Service, n_fd_store_max), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("FileDescriptorStoreMax", "u", bus_property_get_unsigned, offsetof(Service, n_fd_store_max), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("NFileDescriptorStore", "u", bus_property_get_unsigned, offsetof(Service, n_fd_store), 0), SD_BUS_PROPERTY("StatusText", "s", NULL, offsetof(Service, status_text), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("StatusErrno", "i", NULL, offsetof(Service, status_errno), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), SD_BUS_PROPERTY("Result", "s", property_get_result, offsetof(Service, result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), -- cgit v1.2.3-54-g00ecf From cfb571f30fd415304b2f674f1615dc861058c347 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 25 Oct 2015 00:09:44 -0400 Subject: journal: return better error for empty files When reading stuff, we should only return EIO when an actual read error occured, not when we don't like the data for whatever reason. We already return ENODATA for all other kinds of file truncation, hence do the same for the most obvious kind, so that callers know what ENODATA means. --- src/journal/journal-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 12b39dbb27..e00a0d711d 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -2705,7 +2705,7 @@ int journal_file_open( } if (f->last_stat.st_size < (off_t) HEADER_SIZE_MIN) { - r = -EIO; + r = -ENODATA; goto fail; } -- cgit v1.2.3-54-g00ecf From 4f52b822b05c373f40fea1a41ae3ade5d5ff558e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:13:01 +0100 Subject: journalctl: continue operation, even if we run into an invalid file --- src/journal/journalctl.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index a35783e3ff..30ae6e39c3 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -1729,15 +1729,12 @@ static int access_check(sd_journal *j) { SET_FOREACH(code, j->errors, it) { int err; - err = -PTR_TO_INT(code); - assert(err > 0); + err = abs(PTR_TO_INT(code)); if (err == EACCES) continue; - log_warning_errno(err, "Error was encountered while opening journal files: %m"); - if (r == 0) - r = -err; + log_warning_errno(err, "An error was encountered while opening journal files, ignoring: %m"); } return r; -- cgit v1.2.3-54-g00ecf 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(-) 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(-) 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 From 6a37c684695befff2d2ce7e0fcce90e25e2e252c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:47:38 +0100 Subject: machine: when we fail to extract a network interface from the machine state, don't try again If we failed to extract a word, then that's how it is, we shouldn't try to extract it again and again, it's unlikely to work, and we just deadlock. This is a fix-up for 52278ad31d92dffa9029c8a171b9e9348f5dcc8f. --- src/machine/machine.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/machine/machine.c b/src/machine/machine.c index ef5c8551b5..d88b7c90df 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -325,11 +325,11 @@ int machine_load(Machine *m) { r = extract_first_word(&p, &word, NULL, 0); if (r == 0) break; - else if (r == -ENOMEM) + if (r == -ENOMEM) return log_oom(); - else if (r < 0) { + if (r < 0) { log_warning_errno(r, "Failed to parse NETIF: %s", netif); - continue; + break; } if (safe_atoi(word, &ifi) < 0) -- cgit v1.2.3-54-g00ecf From 6ad623a3f77e087e308f334525fd4046811f2a9a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 2 Nov 2015 23:57:21 +0100 Subject: parse-util: introduce parse_ifindex() and make use of it everywhere We have enough places where we parse an ifindex, hence introduce a proper parsing function for it, that verifies all parameters. --- src/basic/parse-util.c | 13 +++++++++++++ src/basic/parse-util.h | 1 + src/libsystemd/sd-device/sd-device.c | 9 ++------- src/libsystemd/sd-login/sd-login.c | 4 +--- src/machine/machine.c | 4 +--- src/network/networkctl.c | 2 +- src/network/networkd-link-bus.c | 2 +- src/resolve-host/resolve-host.c | 5 ++--- 8 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/basic/parse-util.c b/src/basic/parse-util.c index 1ee5783680..b6358c459a 100644 --- a/src/basic/parse-util.c +++ b/src/basic/parse-util.c @@ -81,6 +81,19 @@ int parse_mode(const char *s, mode_t *ret) { return 0; } +int parse_ifindex(const char *s, int *ret) { + int ifi, r; + + r = safe_atoi(s, &ifi); + if (r < 0) + return r; + if (ifi <= 0) + return -EINVAL; + + *ret = ifi; + return 0; +} + int parse_size(const char *t, uint64_t base, uint64_t *size) { /* Soo, sometimes we want to parse IEC binary suffixes, and diff --git a/src/basic/parse-util.h b/src/basic/parse-util.h index 0e56848e26..408690d0b3 100644 --- a/src/basic/parse-util.h +++ b/src/basic/parse-util.h @@ -31,6 +31,7 @@ int parse_boolean(const char *v) _pure_; int parse_pid(const char *s, pid_t* ret_pid); int parse_mode(const char *s, mode_t *ret); +int parse_ifindex(const char *s, int *ret); int parse_size(const char *t, uint64_t base, uint64_t *size); int parse_range(const char *t, unsigned *lower, unsigned *upper); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 0d40bc5c00..0e49262087 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -356,13 +356,10 @@ int device_set_ifindex(sd_device *device, const char *_ifindex) { assert(device); assert(_ifindex); - r = safe_atoi(_ifindex, &ifindex); + r = parse_ifindex(_ifindex, &ifindex); if (r < 0) return r; - if (ifindex <= 0) - return -EINVAL; - r = device_add_property_internal(device, "IFINDEX", _ifindex); if (r < 0) return r; @@ -632,11 +629,9 @@ _public_ int sd_device_new_from_device_id(sd_device **ret, const char *id) { struct ifreq ifr = {}; int ifindex; - r = safe_atoi(&id[1], &ifr.ifr_ifindex); + r = parse_ifindex(&id[1], &ifr.ifr_ifindex); if (r < 0) return r; - else if (ifr.ifr_ifindex <= 0) - return -EINVAL; sk = socket(PF_INET, SOCK_DGRAM, 0); if (sk < 0) diff --git a/src/libsystemd/sd-login/sd-login.c b/src/libsystemd/sd-login/sd-login.c index cd766c3f91..3f2e459825 100644 --- a/src/libsystemd/sd-login/sd-login.c +++ b/src/libsystemd/sd-login/sd-login.c @@ -932,9 +932,7 @@ _public_ int sd_machine_get_ifindices(const char *machine, int **ifindices) { *(char*) (mempcpy(buf, word, l)) = 0; - if (safe_atoi(buf, &ifi) < 0) - continue; - if (ifi <= 0) + if (parse_ifindex(buf, &ifi) < 0) continue; if (!GREEDY_REALLOC(ni, allocated, nr+1)) { diff --git a/src/machine/machine.c b/src/machine/machine.c index d88b7c90df..cbc03640c1 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -332,9 +332,7 @@ int machine_load(Machine *m) { break; } - if (safe_atoi(word, &ifi) < 0) - continue; - if (ifi <= 0) + if (parse_ifindex(word, &ifi) < 0) continue; if (!GREEDY_REALLOC(ni, allocated, nr+1)) { diff --git a/src/network/networkctl.c b/src/network/networkctl.c index 97c1fe6560..ba7e3ba74a 100644 --- a/src/network/networkctl.c +++ b/src/network/networkctl.c @@ -521,7 +521,7 @@ static int link_status_one( assert(rtnl); assert(name); - if (safe_atoi(name, &ifindex) >= 0 && ifindex > 0) + if (parse_ifindex(name, &ifindex) >= 0) r = sd_rtnl_message_new_link(rtnl, &req, RTM_GETLINK, ifindex); else { r = sd_rtnl_message_new_link(rtnl, &req, RTM_GETLINK, 0); diff --git a/src/network/networkd-link-bus.c b/src/network/networkd-link-bus.c index 7fc4510ac9..11b35d6cf8 100644 --- a/src/network/networkd-link-bus.c +++ b/src/network/networkd-link-bus.c @@ -103,7 +103,7 @@ int link_object_find(sd_bus *bus, const char *path, const char *interface, void if (r < 0) return 0; - r = safe_atoi(identifier, &ifindex); + r = parse_ifindex(identifier, &ifindex); if (r < 0) return 0; diff --git a/src/resolve-host/resolve-host.c b/src/resolve-host/resolve-host.c index 57739d1f3e..432e62dd9f 100644 --- a/src/resolve-host/resolve-host.c +++ b/src/resolve-host/resolve-host.c @@ -300,8 +300,7 @@ static int parse_address(const char *s, int *family, union in_addr_union *addres percent = strchr(s, '%'); if (percent) { - r = safe_atoi(percent+1, &ifi); - if (r < 0 || ifi <= 0) { + if (parse_ifindex(percent+1, &ifi) < 0) { ifi = if_nametoindex(percent+1); if (ifi <= 0) return -EINVAL; @@ -521,7 +520,7 @@ static int parse_argv(int argc, char *argv[]) { case 'i': { int ifi; - if (safe_atoi(optarg, &ifi) >= 0 && ifi > 0) + if (parse_ifindex(optarg, &ifi) >= 0) arg_ifindex = ifi; else { ifi = if_nametoindex(optarg); -- cgit v1.2.3-54-g00ecf From 2c1a55cf3fe78bcf728f73487813ddd3ee020a98 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 24 Oct 2015 23:28:07 -0400 Subject: log-generator: add option to generate easily compressible data This is useful to check that compression actually works, and how compression influences file size in the best-case-scenario for compression. (The answer is that not as much as one would hope: there's still a big overhead of the indexing and since every field is compressed separately, even fields that compress very well contribute to the file size. This overhead becomes negligible only for very big fields.) --- src/journal-remote/log-generator.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/journal-remote/log-generator.py b/src/journal-remote/log-generator.py index 9a8fb07c7f..fd6964e758 100755 --- a/src/journal-remote/log-generator.py +++ b/src/journal-remote/log-generator.py @@ -6,6 +6,8 @@ import argparse PARSER = argparse.ArgumentParser() PARSER.add_argument('n', type=int) PARSER.add_argument('--dots', action='store_true') +PARSER.add_argument('--data-size', type=int, default=4000) +PARSER.add_argument('--data-type', choices={'random', 'simple'}) OPTIONS = PARSER.parse_args() template = """\ @@ -38,10 +40,16 @@ facility = 6 src = open('/dev/urandom', 'rb') bytes = 0 +counter = 0 for i in range(OPTIONS.n): message = repr(src.read(2000)) - data = repr(src.read(4000)) + if OPTIONS.data_type == 'random': + data = repr(src.read(OPTIONS.data_size)) + else: + # keep the pattern non-repeating so we get a different blob every time + data = '{:0{}}'.format(counter, OPTIONS.data_size) + counter += 1 entry = template.format(m=m, realtime_ts=realtime_ts, -- cgit v1.2.3-54-g00ecf