From 41f6e627d7cfdf1ea50d5adbd7e118589dbcf8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 11:02:11 +0200 Subject: [PATCH] Make fopen_temporary and fopen_temporary_label unlocked This is partially a refactoring, but also makes many more places use unlocked operations implicitly, i.e. all users of fopen_temporary(). AFAICT, the uses are always for short-lived files which are not shared externally, and are just used within the same context. Locking is not necessary. --- coccinelle/fopen-unlocked.cocci | 14 ++++++++++++++ src/basic/cgroup-util.c | 1 - src/basic/env-file.c | 3 --- src/basic/fileio.c | 11 ++++------- src/basic/mountpoint-util.c | 1 - src/basic/process-util.c | 1 - src/basic/tmpfile-util.c | 7 +++++++ src/core/dbus-service.c | 1 - src/fstab-generator/fstab-generator.c | 1 - src/libsystemd-network/sd-dhcp-lease.c | 2 -- src/locale/keymap-util.c | 2 -- src/login/logind-seat.c | 2 -- src/login/logind-session.c | 2 -- src/login/logind-user.c | 2 -- src/machine/machine.c | 2 -- src/network/networkd-link.c | 2 -- src/network/networkd-manager.c | 2 -- src/resolve/resolved-link.c | 2 -- src/resolve/resolved-resolv-conf.c | 3 --- src/shared/generator.c | 14 ++++++-------- src/shared/mount-util.c | 1 - 21 files changed, 31 insertions(+), 45 deletions(-) diff --git a/coccinelle/fopen-unlocked.cocci b/coccinelle/fopen-unlocked.cocci index 93b993dd556..e6f2bc5681f 100644 --- a/coccinelle/fopen-unlocked.cocci +++ b/coccinelle/fopen-unlocked.cocci @@ -35,3 +35,17 @@ expression f, path, options; + return -ESRCH; + if (r < 0) + return r; +@@ +expression f, path, p; +@@ + r = fopen_temporary(path, &f, &p); + if (r < 0) + return ...; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); +@@ +expression f, g, path, p; +@@ + r = fopen_temporary_label(path, g, &f, &p); + if (r < 0) + return ...; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 11b4e3fce1a..210089688b0 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include diff --git a/src/basic/env-file.c b/src/basic/env-file.c index a1f1308a54a..83767b0a24a 100644 --- a/src/basic/env-file.c +++ b/src/basic/env-file.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "env-file.h" #include "env-util.h" @@ -545,7 +543,6 @@ int write_env_file(const char *fname, char **l) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod_umask(fileno(f), 0644); STRV_FOREACH(i, l) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 4599440b45e..2318f407b87 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -108,7 +108,6 @@ static int write_string_file_atomic( if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod_umask(fileno(f), 0644); r = write_string_stream_ts(f, line, flags, ts); @@ -154,11 +153,9 @@ int write_string_file_ts( assert(!ts); if (flags & WRITE_STRING_FILE_CREATE) { - f = fopen(fn, "we"); - if (!f) { - r = -errno; + r = fopen_unlocked(fn, "we", &f); + if (r < 0) goto fail; - } } else { int fd; @@ -176,9 +173,9 @@ int write_string_file_ts( safe_close(fd); goto fail; } - } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + } if (flags & WRITE_STRING_FILE_DISABLE_BUFFER) setvbuf(f, NULL, _IONBF, 0); diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index 48494320fdb..5ac92931671 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -2,7 +2,6 @@ #include #include -#include #include #include "alloc-util.h" diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 568f400d975..3dc3534e1ab 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index bc92d6a6de2..260443a1d66 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ +#include +#include #include #include "alloc-util.h" @@ -37,6 +39,9 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { return -errno; } + /* This assumes that returned FILE object is short-lived and used within the same single-threaded + * context and never shared externally, hence locking is not necessary. */ + f = fdopen(fd, "w"); if (!f) { unlink_noerrno(t); @@ -45,6 +50,8 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { return -errno; } + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + *_f = f; *_temp_path = t; diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 0f563a66258..4e6709c42e2 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include #include #include "alloc-util.h" diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index d35f2f993e9..28ae48d551a 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -5,7 +5,6 @@ #include #include #include -#include #include "alloc-util.h" #include "fd-util.h" diff --git a/src/libsystemd-network/sd-dhcp-lease.c b/src/libsystemd-network/sd-dhcp-lease.c index a16314a9d37..c089b4278b1 100644 --- a/src/libsystemd-network/sd-dhcp-lease.c +++ b/src/libsystemd-network/sd-dhcp-lease.c @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -832,7 +831,6 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index b8bd181c16a..e238e5a124c 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include @@ -423,7 +422,6 @@ int x11_write_data(Context *c) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fputs("# Written by systemd-localed(8), read by systemd-localed and Xorg. It's\n" diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index b4904c37d5d..f5ffb68238e 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -97,7 +96,6 @@ int seat_save(Seat *s) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 3d3bc8ab1cf..f1efeb0e017 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -214,7 +213,6 @@ int session_save(Session *s) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 045b6f0e17d..8356a9089a9 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -3,7 +3,6 @@ #include #include #include -#include #include "alloc-util.h" #include "bus-common-errors.h" @@ -162,7 +161,6 @@ static int user_save_internal(User *u) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/machine/machine.c b/src/machine/machine.c index 84454ddd864..b916d038d7a 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include "sd-messages.h" @@ -126,7 +125,6 @@ int machine_save(Machine *m) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 3e334c8d29c..4846b13a8b8 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -4,7 +4,6 @@ #include #include #include -#include #include "alloc-util.h" #include "bus-util.h" @@ -4034,7 +4033,6 @@ int link_save(Link *link) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 9075b0a14bd..677f66a478b 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include "sd-daemon.h" @@ -1189,7 +1188,6 @@ static int manager_save(Manager *m) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c index 50f9309f105..f65ce64d178 100644 --- a/src/resolve/resolved-link.c +++ b/src/resolve/resolved-link.c @@ -2,7 +2,6 @@ #include #include -#include #include #include "sd-network.h" @@ -1178,7 +1177,6 @@ int link_save_user(Link *l) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fputs("# This is private data. Do not parse.\n", f); diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 0435791ea0e..dfc9a948e34 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include @@ -359,14 +358,12 @@ int manager_write_resolv_conf(Manager *m) { if (r < 0) return log_warning_errno(r, "Failed to open private resolv.conf file for writing: %m"); - (void) __fsetlocking(f_uplink, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f_uplink), 0644); r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); if (r < 0) return log_warning_errno(r, "Failed to open private stub-resolv.conf file for writing: %m"); - (void) __fsetlocking(f_stub, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f_stub), 0644); r = write_uplink_resolv_conf_contents(f_uplink, dns, domains); diff --git a/src/shared/generator.c b/src/shared/generator.c index ed7f037e918..403c2a6737b 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include "alloc-util.h" @@ -30,23 +29,22 @@ int generator_open_unit_file( const char *unit; FILE *f; + int r; unit = strjoina(dest, "/", name); - f = fopen(unit, "wxe"); - if (!f) { - if (source && errno == EEXIST) - return log_error_errno(errno, + r = fopen_unlocked(unit, "wxe", &f); + if (r < 0) { + if (source && r == -EEXIST) + return log_error_errno(r, "Failed to create unit file %s, as it already exists. Duplicate entry in %s?", unit, source); else - return log_error_errno(errno, + return log_error_errno(r, "Failed to create unit file %s: %m", unit); } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fprintf(f, "# Automatically generated by %s\n\n", program_invocation_short_name); diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 1b50716731e..680c4522ad6 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include