From 2eabcc772b7505b201952cbece332b22dad29647 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 20:45:06 +0200 Subject: [PATCH 1/5] fileio: support writing atomic files with timestamp Let's make sure "ts" is taken into account when writing atomic files, too. --- src/basic/fileio.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 6220d1c11d..be6a5af40c 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -71,7 +71,13 @@ 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 do_fsync) { +static int write_string_file_atomic( + const char *fn, + const char *line, + bool enforce_newline, + bool do_fsync, + struct timespec *ts) { + _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *p = NULL; int r; @@ -85,10 +91,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); + r = write_string_stream_ts(f, line, enforce_newline, ts); if (r >= 0 && do_fsync) r = fflush_sync_and_check(f); - if (r >= 0) { if (rename(p, fn) < 0) r = -errno; @@ -113,8 +118,10 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags 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), - flags & WRITE_STRING_FILE_SYNC); + r = write_string_file_atomic(fn, + line, + !(flags & WRITE_STRING_FILE_AVOID_NEWLINE), + flags & WRITE_STRING_FILE_SYNC, ts); if (r < 0) goto fail; From b18371338345cfbcc1831dea742300d0889b4428 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 20:55:34 +0200 Subject: [PATCH 2/5] fileio: make write_string_stream() accept flags parameter Let's make write_string_stream() and write_string_file() more alike, and pass the same flag set so that we can remove a number of boolean parameters. --- src/basic/fileio.c | 29 +++++++++++++++++------------ src/basic/fileio.h | 6 +++--- src/sleep/sleep.c | 2 +- src/test/test-clock.c | 2 +- src/test/test-fileio.c | 10 +++++----- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index be6a5af40c..e95036d19b 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -52,13 +52,17 @@ #define READ_FULL_BYTES_MAX (4U*1024U*1024U) -int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, struct timespec *ts) { +int write_string_stream_ts( + FILE *f, + const char *line, + WriteStringFileFlags flags, + struct timespec *ts) { assert(f); assert(line); fputs(line, f); - if (enforce_newline && !endswith(line, "\n")) + if (!(flags & WRITE_STRING_FILE_AVOID_NEWLINE) && !endswith(line, "\n")) fputc('\n', f); if (ts) { @@ -74,8 +78,7 @@ int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, stru static int write_string_file_atomic( const char *fn, const char *line, - bool enforce_newline, - bool do_fsync, + WriteStringFileFlags flags, struct timespec *ts) { _cleanup_fclose_ FILE *f = NULL; @@ -91,8 +94,8 @@ static int write_string_file_atomic( (void) fchmod_umask(fileno(f), 0644); - r = write_string_stream_ts(f, line, enforce_newline, ts); - if (r >= 0 && do_fsync) + r = write_string_stream_ts(f, line, flags, ts); + if (r >= 0 && (flags & WRITE_STRING_FILE_SYNC)) r = fflush_sync_and_check(f); if (r >= 0) { if (rename(p, fn) < 0) @@ -105,7 +108,12 @@ static int write_string_file_atomic( return r; } -int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags flags, struct timespec *ts) { +int write_string_file_ts( + const char *fn, + const char *line, + WriteStringFileFlags flags, + struct timespec *ts) { + _cleanup_fclose_ FILE *f = NULL; int q, r; @@ -118,10 +126,7 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags 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), - flags & WRITE_STRING_FILE_SYNC, ts); + r = write_string_file_atomic(fn, line, flags, ts); if (r < 0) goto fail; @@ -154,7 +159,7 @@ int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags } } - r = write_string_stream_ts(f, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE), ts); + r = write_string_stream_ts(f, line, flags, ts); if (r < 0) goto fail; diff --git a/src/basic/fileio.h b/src/basic/fileio.h index b4238746d6..c4a42eb8e9 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -36,9 +36,9 @@ typedef enum { WRITE_STRING_FILE_SYNC = 1<<4, } WriteStringFileFlags; -int write_string_stream_ts(FILE *f, const char *line, bool enforce_newline, struct timespec *ts); -static inline int write_string_stream(FILE *f, const char *line, bool enforce_newline) { - return write_string_stream_ts(f, line, enforce_newline, NULL); +int write_string_stream_ts(FILE *f, const char *line, WriteStringFileFlags flags, struct timespec *ts); +static inline int write_string_stream(FILE *f, const char *line, WriteStringFileFlags flags) { + return write_string_stream_ts(f, line, flags, NULL); } int write_string_file_ts(const char *fn, const char *line, WriteStringFileFlags flags, struct timespec *ts); static inline int write_string_file(const char *fn, const char *line, WriteStringFileFlags flags) { diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index 4c171a2fe5..4d774c90b3 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -66,7 +66,7 @@ static int write_state(FILE **f, char **states) { STRV_FOREACH(state, states) { int k; - k = write_string_stream(*f, *state, true); + k = write_string_stream(*f, *state, 0); if (k == 0) return 0; log_debug_errno(k, "Failed to write '%s' to /sys/power/state: %m", diff --git a/src/test/test-clock.c b/src/test/test-clock.c index 7d97328416..f246ae8385 100644 --- a/src/test/test-clock.c +++ b/src/test/test-clock.c @@ -66,7 +66,7 @@ static void test_clock_is_localtime(void) { log_info("%s", scenarios[i].contents); rewind(f); ftruncate(fd, 0); - assert_se(write_string_stream(f, scenarios[i].contents, false) == 0); + assert_se(write_string_stream(f, scenarios[i].contents, WRITE_STRING_FILE_AVOID_NEWLINE) == 0); assert_se(clock_is_localtime(adjtime) == scenarios[i].expected_result); } diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index b1d688c89e..31bf84f6e6 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -233,7 +233,7 @@ static void test_merge_env_file(void) { "zzz=${one:+replacement}\n" "zzzz=${foobar:-${nothing}}\n" "zzzzz=${nothing:+${nothing}}\n" - , false); + , WRITE_STRING_FILE_AVOID_NEWLINE); assert(r >= 0); r = merge_env_file(&a, NULL, t); @@ -302,7 +302,7 @@ static void test_merge_env_file_invalid(void) { ";comment2=comment2\n" "#\n" "\n\n" /* empty line */ - , false); + , WRITE_STRING_FILE_AVOID_NEWLINE); assert(r >= 0); r = merge_env_file(&a, NULL, t); @@ -414,12 +414,12 @@ static void test_write_string_stream(void) { f = fdopen(fd, "r"); assert_se(f); - assert_se(write_string_stream(f, "boohoo", true) < 0); + assert_se(write_string_stream(f, "boohoo", 0) < 0); f = freopen(fn, "r+", f); assert_se(f); - assert_se(write_string_stream(f, "boohoo", true) == 0); + assert_se(write_string_stream(f, "boohoo", 0) == 0); rewind(f); assert_se(fgets(buf, sizeof(buf), f)); @@ -428,7 +428,7 @@ static void test_write_string_stream(void) { f = freopen(fn, "w+", f); assert_se(f); - assert_se(write_string_stream(f, "boohoo", false) == 0); + assert_se(write_string_stream(f, "boohoo", WRITE_STRING_FILE_AVOID_NEWLINE) == 0); rewind(f); assert_se(fgets(buf, sizeof(buf), f)); From 9dd1b1e8694c3cbfaf64bc9d93ff6e7a61694a49 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 20:59:39 +0200 Subject: [PATCH 3/5] fileio: move fsync() logic into write_string_stream_ts() That way, write_string_stream_ts() becomes more powerful, and we can remove duplicate code from write_string_file_atomic() and write_string_file_ts(). --- src/basic/fileio.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index e95036d19b..7d69fae709 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -72,7 +72,10 @@ int write_string_stream_ts( return -errno; } - return fflush_and_check(f); + if (flags & WRITE_STRING_FILE_SYNC) + return fflush_sync_and_check(f); + else + return fflush_and_check(f); } static int write_string_file_atomic( @@ -95,16 +98,18 @@ static int write_string_file_atomic( (void) fchmod_umask(fileno(f), 0644); r = write_string_stream_ts(f, line, flags, ts); - if (r >= 0 && (flags & WRITE_STRING_FILE_SYNC)) - r = fflush_sync_and_check(f); - if (r >= 0) { - if (rename(p, fn) < 0) - r = -errno; + if (r < 0) + goto fail; + + if (rename(p, fn) < 0) { + r = -errno; + goto fail; } - if (r < 0) - (void) unlink(p); + return 0; +fail: + (void) unlink(p); return r; } @@ -163,12 +168,6 @@ int write_string_file_ts( 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: From ff0e7e05c9904b4120868e96216b123c1b798aa4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 21:03:33 +0200 Subject: [PATCH 4/5] fileio: try to read one byte too much in read_full_stream() Let's read one byte more than the file size we read from stat() on the first fread() invocation. That way, the first read() will already be short and indicate eof to fread(). This is a minor optimization, and replaces #3908. --- 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 7d69fae709..e7cd03b939 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -270,11 +270,11 @@ int read_full_stream(FILE *f, char **contents, size_t *size) { if (st.st_size > READ_FULL_BYTES_MAX) return -E2BIG; - /* Start with the right file size, but be prepared for - * files from /proc which generally report a file size - * of 0 */ + /* Start with the right file size, but be prepared for files from /proc which generally report a file + * size of 0. Note that we increase the size to read here by one, so that the first read attempt + * already makes us notice the EOF. */ if (st.st_size > 0) - n = st.st_size; + n = st.st_size + 1; } l = 0; From 5a89faf0e05e732d8de47d48f1fcfa2a398c53d8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 21:05:03 +0200 Subject: [PATCH 5/5] fileio: initialize errno to zero before we do fread() if there was something in the read buffer already errno might not be set on error, let's detect that case. --- src/basic/fileio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index e7cd03b939..690c67cf62 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -287,12 +287,13 @@ int read_full_stream(FILE *f, char **contents, size_t *size) { return -ENOMEM; buf = t; + errno = 0; k = fread(buf + l, 1, n - l, f); if (k > 0) l += k; if (ferror(f)) - return -errno; + return errno > 0 ? -errno : -EIO; if (feof(f)) break;