all: cleanup close() handling and clarify nm_close()/nm_close_with_error()

Cleanup the handling of close().

First of all, closing an invalid (non-negative) file descriptor (EBADF) is
always a serious bug. We want to catch that. Hence, we should use nm_close()
(or nm_close_with_error()) which asserts against such bugs. Don't ever use
close() directly, to get that additional assertion.

Also, our nm_close() handles EINTR internally and correctly. Recent
POSIX defines that on EINTR the close should be retried. On Linux,
that is never correct. After close() returns, the file descriptor is
always closed (or invalid). nm_close() gets this right, and pretends
that EINTR is a success (without retrying).

The majority of our file descriptors are sockets, etc. That means,
often an error from close isn't something that we want to handle. Adjust
nm_close() to return no error and preserve the caller's errno. That is
the appropriate reaction to error (ignoring it) in most of our cases.

And error from close may mean that there was an IO error (except EINTR
and EBADF). In a few cases, we may want to handle that. For those
cases we have nm_close_with_error().

TL;DR: use almost always nm_close(). Unless you want to handle the error
code, then use nm_close_with_error(). Never use close() directly.

There is much reading on the internet about handling errors of close and
in particular EINTR. See the following links:

https://lwn.net/Articles/576478/
https://askcodes.net/coding/what-to-do-if-a-posix-close-call-fails-
https://www.austingroupbugs.net/view.php?id=529
https://sourceware.org/bugzilla/show_bug.cgi?id=14627
https://news.ycombinator.com/item?id=3363819
https://peps.python.org/pep-0475/

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.

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;

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;
     }
 

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

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)