diff --git a/contrib/scripts/checkpatch.pl b/contrib/scripts/checkpatch.pl index 3c53ffae06..797caac847 100755 --- a/contrib/scripts/checkpatch.pl +++ b/contrib/scripts/checkpatch.pl @@ -206,6 +206,7 @@ complain ("Define setting properties with _nm_setting_property_define_direct_*() complain ("Use nm_g_array_{index,first,last,index_p}() instead of g_array_index(), as it nm_assert()s for valid element size and out-of-bound access") if $line =~ /\bg_array_index\b/; complain ("Use spaces instead of tabs") if $line =~ /\t/; complain ("Prefer implementing private pointers via _NM_GET_PRIVATE() or _NM_GET_PRIVATE_PTR() (the latter, if the private data has an opqaue pointer in the header file)") if $line =~ /\b(g_type_class_add_private|G_TYPE_INSTANCE_GET_PRIVATE)\b/; +complain ("Don't use close()/g_close(). Instead, use nm_close() (or nm_close_with_error()).") if $line =~ /\b(close|g_close)\b *\(/; complain ("Use nm_memdup() instead of g_memdup(). The latter has a size argument of type guint") if $line =~ /\bg_memdup\b/; # Further on we process stuff without comments. diff --git a/src/core/main-utils.c b/src/core/main-utils.c index aec73e12db..b18ae311ed 100644 --- a/src/core/main-utils.c +++ b/src/core/main-utils.c @@ -80,6 +80,7 @@ nm_main_utils_write_pidfile(const char *pidfile) int fd; int errsv; gboolean success = FALSE; + int r; if ((fd = open(pidfile, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 00644)) < 0) { errsv = errno; @@ -94,9 +95,9 @@ nm_main_utils_write_pidfile(const char *pidfile) } else success = TRUE; - if (nm_close(fd)) { - errsv = errno; - fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(errsv)); + r = nm_close_with_error(fd); + if (r < 0) { + fprintf(stderr, _("Closing %s failed: %s\n"), pidfile, nm_strerror_native(-r)); } return success; diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index d2d48bb99a..ac137545c7 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -5608,12 +5608,14 @@ sysctl_set_internal(NMPlatform *platform, const char *path, const char *value) { - int fd, tries; + int fd; + int tries; gssize nwrote; gssize len; char *actual; gs_free char *actual_free = NULL; int errsv; + int r; if (dirfd < 0) { pathid = path; @@ -5707,18 +5709,13 @@ sysctl_set_internal(NMPlatform *platform, _LOGE("sysctl: failed to set '%s' to '%s' after three attempts", path, value); } - if (nwrote < len - 1) { - if (nm_close(fd) != 0) { - if (errsv != 0) - errno = errsv; - } else if (errsv != 0) - errno = errsv; - else - errno = EIO; - return FALSE; - } - if (nm_close(fd) != 0) { - /* errno is already properly set. */ + r = nm_close_with_error(fd); + if (r < 0 || nwrote < len - 1) { + if (errsv == 0) { + /* propagate the error from nm_close_with_error(). */ + errsv = (r < 0) ? -r : EIO; + } + errno = errsv; return FALSE; } diff --git a/src/libnm-platform/wifi/nm-wifi-utils-wext.c b/src/libnm-platform/wifi/nm-wifi-utils-wext.c index 678d71fe60..eac3c929bc 100644 --- a/src/libnm-platform/wifi/nm-wifi-utils-wext.c +++ b/src/libnm-platform/wifi/nm-wifi-utils-wext.c @@ -90,7 +90,7 @@ dispose(GObject *object) { NMWifiUtilsWext *wext = NM_WIFI_UTILS_WEXT(object); - wext->fd = nm_close(wext->fd); + nm_clear_fd(&wext->fd); } static gboolean diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index 526b596e63..fbfa619980 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -957,38 +957,6 @@ _NM_IN_STRSET_EVAL_op_streq(const char *x1, const char *x) /*****************************************************************************/ -/** - * nm_close: - * - * Like close() but throws an assertion if the input fd is - * invalid. Closing an invalid fd is a programming error, so - * it's better to catch it early. - */ -static inline int -nm_close(int fd) -{ - int r; - - r = close(fd); - nm_assert(r != -1 || fd < 0 || errno != EBADF); - return r; -} - -static inline bool -nm_clear_fd(int *p_fd) -{ - int fd; - - if (!p_fd || (fd = *p_fd) < 0) - return false; - - *p_fd = -1; - nm_close(fd); - return true; -} - -/*****************************************************************************/ - /* Note: @value is only evaluated when *out_val is present. * Thus, * NM_SET_OUT (out_str, g_strdup ("hallo")); @@ -1066,13 +1034,96 @@ _nm_auto_protect_errno(const int *p_saved_errno) /*****************************************************************************/ +/** + * nm_close_with_error: + * + * Wrapper around close(). + * + * This fails an nm_assert() for EBADF with a non-negative file descriptor. Trying + * to close an invalid file descriptor is always a serious bug. Never use close() + * directly, because we want to catch such bugs. + * + * This also suppresses any EINTR and pretends success. That is appropriate + * on Linux (but not necessarily on other POSIX systems). + * + * In no case is it appropriate to use @fd afterwards (or retry). + * + * This function returns 0 on success, or a negative errno value. + * On success, errno is undefined afterwards. On failure, errno is + * the same as the (negative) return value. + * + * In the common case, when you don't intend to handle the error from close(), + * prefer nm_close() over nm_close_with_error(). Never use close() directly. + * + * The function is also async-signal-safe (unless an assertion fails). + * + * Returns: 0 on success or the negative errno from close(). + */ +static inline int +nm_close_with_error(int fd) +{ + int r; + + r = close(fd); + + if (r != 0) { + int errsv = errno; + + nm_assert(r == -1); + nm_assert((fd < 0 && errsv == EBADF) || (fd >= 0 && errsv != EBADF)); + + if (errsv == EINTR) { + /* There isn't really much we can do about EINTR. On Linux, always this means + * the FD was closed. On some POSIX systems that may be different and retry + * would be appropriate. + * + * Whether there was any IO error is unknown. Assume not and signal success. */ + return 0; + } + + return -errsv; + } + + return 0; +} + +/** + * nm_close: + * + * Wrapper around nm_close_with_error(), which ignores any error and preserves the + * caller's errno. + * + * We usually don't care about errors from close, so this is usually preferable over + * nm_close_with_error(). Never use close() directly. + * + * Everything from nm_close_with_error() applies. + */ +static inline void +nm_close(int fd) +{ + NM_AUTO_PROTECT_ERRNO(errsv); + + nm_close_with_error(fd); +} + +static inline bool +nm_clear_fd(int *p_fd) +{ + int fd; + + if (!p_fd || (fd = *p_fd) < 0) + return false; + + *p_fd = -1; + nm_close(fd); + return true; +} + static inline void _nm_auto_close(int *pfd) { - if (*pfd >= 0) { - NM_AUTO_PROTECT_ERRNO(errsv); - (void) nm_close(*pfd); - } + if (*pfd >= 0) + nm_close(*pfd); } #define nm_auto_close nm_auto(_nm_auto_close)