From dce892acef1c5316fb98f7f5ea287bc74f935ca3 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 17 Aug 2017 17:21:34 +0100 Subject: [PATCH 1/3] localed: don't remove xorg.conf.d/00-keyboard.conf on failures One of the benefits of updating a file "atomically", is to avoid losing the old version. For example, if we run out of disk space half-way through. Fix localed to enjoy this benefit. --- src/locale/keymap-util.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index ec48e8ba6d..5cefeb6135 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -394,8 +394,6 @@ int x11_write_data(Context *c) { return 0; fail: - (void) unlink("/etc/X11/xorg.conf.d/00-keyboard.conf"); - if (temp_path) (void) unlink(temp_path); From 0675e94ab53237ad27bfba929c7490bdd2215cf1 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 17 Aug 2017 17:09:44 +0100 Subject: [PATCH 2/3] "Don't fear the fsync()" For files which are vital to boot 1. Avoid opening any window where power loss will zero them out or worse. I know app developers all coded to the ext3 implementation, but the only formal documentation we have says we're broken if we actually rely on it. E.g. * `man mount`, search for `auto_da_alloc`. * http://www.linux-mtd.infradead.org/faq/ubifs.html#L_atomic_change * https://thunk.org/tytso/blog/2009/03/15/dont-fear-the-fsync/ 2. If we tell the kernel we're interested in writing them to disk, it will tell us if that fails. So at minimum, this means we play our part in notifying the user about errors. I refactored error-handling in `udevadm-hwdb` a little. It turns out I did exactly the same as had already been done in the `systemd-hwdb` version, i.e. commit d702dcd. --- src/basic/fileio.c | 32 +++++++++++++++++++++++++++-- src/basic/fileio.h | 10 ++++++---- src/boot/bootctl.c | 14 +++++++++---- src/firstboot/firstboot.c | 8 +++++--- src/hwdb/hwdb.c | 25 ++++++++++++++--------- src/locale/keymap-util.c | 2 +- src/sysusers/sysusers.c | 14 +++++++++---- src/udev/udevadm-hwdb.c | 42 ++++++++++++++++++++------------------- 8 files changed, 100 insertions(+), 47 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index a025038588..277aea51a9 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -71,7 +71,7 @@ int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, stru return fflush_and_check(f); } -static int write_string_file_atomic(const char *fn, const char *line, bool enforce_newline) { +static int write_string_file_atomic(const char *fn, const char *line, bool enforce_newline, bool sync) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *p = NULL; int r; @@ -86,6 +86,9 @@ static int write_string_file_atomic(const char *fn, const char *line, bool enfor (void) fchmod_umask(fileno(f), 0644); r = write_string_stream(f, line, enforce_newline); + if (r >= 0 && sync) + r = fflush_sync_and_check(f); + if (r >= 0) { if (rename(p, fn) < 0) r = -errno; @@ -104,10 +107,14 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags assert(fn); assert(line); + /* We don't know how to verify whether the file contents is on-disk. */ + assert(!((flags & WRITE_STRING_FILE_SYNC) && (flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE))); + if (flags & WRITE_STRING_FILE_ATOMIC) { assert(flags & WRITE_STRING_FILE_CREATE); - r = write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE)); + r = write_string_file_atomic(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE), + flags & WRITE_STRING_FILE_SYNC); if (r < 0) goto fail; @@ -144,6 +151,12 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags if (r < 0) goto fail; + if (flags & WRITE_STRING_FILE_SYNC) { + r = fflush_sync_and_check(f); + if (r < 0) + return r; + } + return 0; fail: @@ -1126,6 +1139,21 @@ int fflush_and_check(FILE *f) { return 0; } +int fflush_sync_and_check(FILE *f) { + int r; + + assert(f); + + r = fflush_and_check(f); + if (r < 0) + return r; + + if (fsync(fileno(f)) < 0) + return -errno; + + return 0; +} + /* This is much like mkostemp() but is subject to umask(). */ int mkostemp_safe(char *pattern) { _cleanup_umask_ mode_t u = 0; diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 6098562265..b4238746d6 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -29,10 +29,11 @@ #include "time-util.h" typedef enum { - WRITE_STRING_FILE_CREATE = 1, - WRITE_STRING_FILE_ATOMIC = 2, - WRITE_STRING_FILE_AVOID_NEWLINE = 4, - WRITE_STRING_FILE_VERIFY_ON_FAILURE = 8, + WRITE_STRING_FILE_CREATE = 1<<0, + WRITE_STRING_FILE_ATOMIC = 1<<1, + WRITE_STRING_FILE_AVOID_NEWLINE = 1<<2, + WRITE_STRING_FILE_VERIFY_ON_FAILURE = 1<<3, + WRITE_STRING_FILE_SYNC = 1<<4, } WriteStringFileFlags; int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, struct timespec *ts); @@ -77,6 +78,7 @@ int search_and_fopen_nulstr(const char *path, const char *mode, const char *root } else int fflush_and_check(FILE *f); +int fflush_sync_and_check(FILE *f); int fopen_temporary(const char *path, FILE **_f, char **_temp_path); int mkostemp_safe(char *pattern); diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index 233bc80292..85f3b42c48 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -539,12 +539,18 @@ static int copy_file_with_version_check(const char *from, const char *to, bool f r = copy_bytes(fd_from, fd_to, (uint64_t) -1, COPY_REFLINK); if (r < 0) { - unlink(t); - return log_error_errno(errno, "Failed to copy data from \"%s\" to \"%s\": %m", from, t); + (void) unlink(t); + return log_error_errno(r, "Failed to copy data from \"%s\" to \"%s\": %m", from, t); } (void) copy_times(fd_from, fd_to); + r = fsync(fd_to); + if (r < 0) { + (void) unlink_noerrno(t); + return log_error_errno(errno, "Failed to copy data from \"%s\" to \"%s\": %m", from, t); + } + r = renameat(AT_FDCWD, t, AT_FDCWD, to); if (r < 0) { (void) unlink_noerrno(t); @@ -912,7 +918,7 @@ static int install_loader_config(const char *esp_path) { r = sd_id128_get_machine(&machine_id); if (r < 0) - return log_error_errno(r, "Failed to get machine did: %m"); + return log_error_errno(r, "Failed to get machine id: %m"); p = strjoina(esp_path, "/loader/loader.conf"); @@ -932,7 +938,7 @@ static int install_loader_config(const char *esp_path) { fprintf(f, "#timeout 3\n"); fprintf(f, "default %s-*\n", sd_id128_to_string(machine_id, machine_string)); - r = fflush_and_check(f); + r = fflush_sync_and_check(f); if (r < 0) return log_error_errno(r, "Failed to write \"%s\": %m", p); diff --git a/src/firstboot/firstboot.c b/src/firstboot/firstboot.c index b3578d3e16..586674d458 100644 --- a/src/firstboot/firstboot.c +++ b/src/firstboot/firstboot.c @@ -411,7 +411,8 @@ static int process_hostname(void) { return 0; mkdir_parents(etc_hostname, 0755); - r = write_string_file(etc_hostname, arg_hostname, WRITE_STRING_FILE_CREATE); + r = write_string_file(etc_hostname, arg_hostname, + WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_SYNC); if (r < 0) return log_error_errno(r, "Failed to write %s: %m", etc_hostname); @@ -432,7 +433,8 @@ static int process_machine_id(void) { return 0; mkdir_parents(etc_machine_id, 0755); - r = write_string_file(etc_machine_id, sd_id128_to_string(arg_machine_id, id), WRITE_STRING_FILE_CREATE); + r = write_string_file(etc_machine_id, sd_id128_to_string(arg_machine_id, id), + WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_SYNC); if (r < 0) return log_error_errno(r, "Failed to write machine id: %m"); @@ -503,7 +505,7 @@ static int write_root_shadow(const char *path, const struct spwd *p) { if (putspent(p, f) != 0) return errno > 0 ? -errno : -EIO; - return fflush_and_check(f); + return fflush_sync_and_check(f); } static int process_root_password(void) { diff --git a/src/hwdb/hwdb.c b/src/hwdb/hwdb.c index 793398ca68..ead4e1770b 100644 --- a/src/hwdb/hwdb.c +++ b/src/hwdb/hwdb.c @@ -403,14 +403,14 @@ static int trie_store(struct trie *trie, const char *filename) { t.strings_off = sizeof(struct trie_header_f); trie_store_nodes_size(&t, trie->root); - r = fopen_temporary(filename , &t.f, &filename_tmp); + r = fopen_temporary(filename, &t.f, &filename_tmp); if (r < 0) return r; fchmod(fileno(t.f), 0444); /* write nodes */ if (fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET) < 0) - goto error; + goto error_fclose; root_off = trie_store_nodes(&t, trie->root); h.nodes_root_off = htole64(root_off); @@ -425,13 +425,20 @@ static int trie_store(struct trie *trie, const char *filename) { size = ftello(t.f); h.file_size = htole64(size); if (fseeko(t.f, 0, SEEK_SET) < 0) - goto error; - + goto error_fclose; fwrite(&h, sizeof(struct trie_header_f), 1, t.f); - if (fclose(t.f) < 0 || rename(filename_tmp, filename) < 0) { - unlink_noerrno(filename_tmp); - return -errno; - } + + if (ferror(t.f)) + goto error_fclose; + if (fflush(t.f) < 0) + goto error_fclose; + if (fsync(fileno(t.f)) < 0) + goto error_fclose; + if (rename(filename_tmp, filename) < 0) + goto error_fclose; + + /* write succeeded */ + fclose(t.f); log_debug("=== trie on-disk ==="); log_debug("size: %8"PRIi64" bytes", size); @@ -446,7 +453,7 @@ static int trie_store(struct trie *trie, const char *filename) { log_debug("strings start: %8"PRIu64, t.strings_off); return 0; - error: + error_fclose: r = -errno; fclose(t.f); unlink(filename_tmp); diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index 5cefeb6135..7068603f0f 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -382,7 +382,7 @@ int x11_write_data(Context *c) { fputs_unlocked("EndSection\n", f); - r = fflush_and_check(f); + r = fflush_sync_and_check(f); if (r < 0) goto fail; diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index fbe51a6ed5..448f584733 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -233,9 +233,15 @@ static int make_backup(const char *target, const char *x) { if (futimens(fileno(dst), ts) < 0) log_warning_errno(errno, "Failed to fix access and modification time of %s: %m", backup); - if (rename(temp, backup) < 0) + r = fflush_sync_and_check(dst); + if (r < 0) goto fail; + if (rename(temp, backup) < 0) { + r = -errno; + goto fail; + } + return 0; fail: @@ -532,7 +538,7 @@ static int write_temporary_shadow(const char *shadow_path, FILE **tmpfile, char return errno ? -errno : -EIO; } - r = fflush_and_check(shadow); + r = fflush_sync_and_check(shadow); if (r < 0) return r; @@ -616,7 +622,7 @@ static int write_temporary_group(const char *group_path, FILE **tmpfile, char ** group_changed = true; } - r = fflush_and_check(group); + r = fflush_sync_and_check(group); if (r < 0) return r; @@ -693,7 +699,7 @@ static int write_temporary_gshadow(const char * gshadow_path, FILE **tmpfile, ch group_changed = true; } - r = fflush_and_check(gshadow); + r = fflush_sync_and_check(gshadow); if (r < 0) return r; diff --git a/src/udev/udevadm-hwdb.c b/src/udev/udevadm-hwdb.c index 69b0b9025c..7688f8192b 100644 --- a/src/udev/udevadm-hwdb.c +++ b/src/udev/udevadm-hwdb.c @@ -364,18 +364,14 @@ static int trie_store(struct trie *trie, const char *filename) { t.strings_off = sizeof(struct trie_header_f); trie_store_nodes_size(&t, trie->root); - err = fopen_temporary(filename , &t.f, &filename_tmp); + err = fopen_temporary(filename, &t.f, &filename_tmp); if (err < 0) return err; fchmod(fileno(t.f), 0444); /* write nodes */ - err = fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET); - if (err < 0) { - fclose(t.f); - unlink_noerrno(filename_tmp); - return -errno; - } + if (fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET) < 0) + goto error_fclose; root_off = trie_store_nodes(&t, trie->root); h.nodes_root_off = htole64(root_off); pos = ftello(t.f); @@ -388,21 +384,21 @@ static int trie_store(struct trie *trie, const char *filename) { /* write header */ size = ftello(t.f); h.file_size = htole64(size); - err = fseeko(t.f, 0, SEEK_SET); - if (err < 0) { - fclose(t.f); - unlink_noerrno(filename_tmp); - return -errno; - } + if (fseeko(t.f, 0, SEEK_SET < 0)) + goto error_fclose; fwrite(&h, sizeof(struct trie_header_f), 1, t.f); - err = ferror(t.f); - if (err) - err = -errno; + + if (ferror(t.f)) + goto error_fclose; + if (fflush(t.f) < 0) + goto error_fclose; + if (fsync(fileno(t.f)) < 0) + goto error_fclose; + if (rename(filename_tmp, filename) < 0) + goto error_fclose; + + /* write succeeded */ fclose(t.f); - if (err < 0 || rename(filename_tmp, filename) < 0) { - unlink_noerrno(filename_tmp); - return err < 0 ? err : -errno; - } log_debug("=== trie on-disk ==="); log_debug("size: %8"PRIi64" bytes", size); @@ -417,6 +413,12 @@ static int trie_store(struct trie *trie, const char *filename) { log_debug("strings start: %8"PRIu64, t.strings_off); return 0; + + error_fclose: + err = -errno; + fclose(t.f); + unlink(filename_tmp); + return err; } static int insert_data(struct trie *trie, struct udev_list *match_list, From 265710c2055254a98ed6dcd6aa172ca509a33553 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 29 Aug 2017 10:56:32 +0100 Subject: [PATCH 3/3] fileio: rename function parameter to avoid masking global symbol > glibc exports a function called sync(), we should probably avoid > overloading that as a variable here locally (gcc even used to warn about > that, not sure why it doesn't anymore), to avoid confusion around what > "if (sync)" actually means --- src/basic/fileio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 277aea51a9..6220d1c11d 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -71,7 +71,7 @@ int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, stru return fflush_and_check(f); } -static int write_string_file_atomic(const char *fn, const char *line, bool enforce_newline, bool sync) { +static int write_string_file_atomic(const char *fn, const char *line, bool enforce_newline, bool do_fsync) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *p = NULL; int r; @@ -86,7 +86,7 @@ static int write_string_file_atomic(const char *fn, const char *line, bool enfor (void) fchmod_umask(fileno(f), 0644); r = write_string_stream(f, line, enforce_newline); - if (r >= 0 && sync) + if (r >= 0 && do_fsync) r = fflush_sync_and_check(f); if (r >= 0) { @@ -107,8 +107,8 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags assert(fn); assert(line); - /* We don't know how to verify whether the file contents is on-disk. */ - assert(!((flags & WRITE_STRING_FILE_SYNC) && (flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE))); + /* We don't know how to verify whether the file contents was already on-disk. */ + assert(!((flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE) && (flags & WRITE_STRING_FILE_SYNC))); if (flags & WRITE_STRING_FILE_ATOMIC) { assert(flags & WRITE_STRING_FILE_CREATE);