Merge branch 'socket-nonblock' into 'main'

gsocket/inotify/gwakeup: Use SOCK_NONBLOCK and O_NONBLOCK to avoid fcntl() syscalls where possible

See merge request GNOME/glib!3347

7 files changed, 122 insertions, 66 deletions

diff --git a/gio/gsocket.c b/gio/gsocket.c
index 2dc005b63..22a8dd7ec 100644
--- a/gio/gsocket.c
+++ b/gio/gsocket.c
@@ -587,7 +587,38 @@ g_socket_details_from_fd (GSocket *socket)
 	       socket_strerror (errsv));
 }
 
-/* Wrapper around socket() that is shared with gnetworkmonitornetlink.c */
+static void
+socket_set_nonblock (int fd)
+{
+#ifndef G_OS_WIN32
+  GError *error = NULL;
+#else
+  gulong arg;
+#endif
+
+  /* Always use native nonblocking sockets, as Windows sets sockets to
+   * nonblocking automatically in certain operations. This way we make
+   * things work the same on all platforms.
+   */
+#ifndef G_OS_WIN32
+  if (!g_unix_set_fd_nonblocking (fd, TRUE, &error))
+    {
+      g_warning ("Error setting socket to nonblocking mode: %s", error->message);
+      g_clear_error (&error);
+    }
+#else
+  arg = TRUE;
+
+  if (ioctlsocket (fd, FIONBIO, &arg) == SOCKET_ERROR)
+    {
+      int errsv = get_socket_errno ();
+      g_warning ("Error setting socket status flags: %s", socket_strerror (errsv));
+    }
+#endif
+}
+
+/* Wrapper around socket() that is shared with gnetworkmonitornetlink.c.
+ * It always sets SOCK_CLOEXEC | SOCK_NONBLOCK. */
 gint
 g_socket (gint     domain,
           gint     type,
@@ -596,13 +627,13 @@ g_socket (gint     domain,
 {
   int fd, errsv;
 
-#ifdef SOCK_CLOEXEC
-  fd = socket (domain, type | SOCK_CLOEXEC, protocol);
+#if defined(SOCK_CLOEXEC) && defined(SOCK_NONBLOCK)
+  fd = socket (domain, type | SOCK_CLOEXEC | SOCK_NONBLOCK, protocol);
   errsv = errno;
   if (fd != -1)
     return fd;
 
-  /* It's possible that libc has SOCK_CLOEXEC but the kernel does not */
+  /* It's possible that libc has SOCK_CLOEXEC and/or SOCK_NONBLOCK but the kernel does not */
   if (fd < 0 && (errsv == EINVAL || errsv == EPROTOTYPE))
 #endif
     fd = socket (domain, type, protocol);
@@ -644,9 +675,13 @@ g_socket (gint     domain,
     }
 #endif
 
+  /* Ensure the socket is non-blocking. */
+  socket_set_nonblock (fd);
+
   return fd;
 }
 
+/* Returned socket has SOCK_CLOEXEC | SOCK_NONBLOCK set. */
 static gint
 g_socket_create_socket (GSocketFamily   family,
 			GSocketType     type,
@@ -696,44 +731,22 @@ g_socket_constructed (GObject *object)
   GSocket *socket = G_SOCKET (object);
 
   if (socket->priv->fd >= 0)
-    /* create socket->priv info from the fd */
-    g_socket_details_from_fd (socket);
-
+    {
+      /* create socket->priv info from the fd and ensure it’s non-blocking */
+      g_socket_details_from_fd (socket);
+      socket_set_nonblock (socket->priv->fd);
+    }
   else
-    /* create the fd from socket->priv info */
-    socket->priv->fd = g_socket_create_socket (socket->priv->family,
-					       socket->priv->type,
-					       socket->priv->protocol,
-					       &socket->priv->construct_error);
+    {
+      /* create the fd from socket->priv info; this sets it non-blocking by construction */
+      socket->priv->fd = g_socket_create_socket (socket->priv->family,
+					         socket->priv->type,
+					         socket->priv->protocol,
+					         &socket->priv->construct_error);
+    }
 
   if (socket->priv->fd != -1)
     {
-#ifndef G_OS_WIN32
-      GError *error = NULL;
-#else
-      gulong arg;
-#endif
-
-      /* Always use native nonblocking sockets, as Windows sets sockets to
-       * nonblocking automatically in certain operations. This way we make
-       * things work the same on all platforms.
-       */
-#ifndef G_OS_WIN32
-      if (!g_unix_set_fd_nonblocking (socket->priv->fd, TRUE, &error))
-        {
-          g_warning ("Error setting socket nonblocking: %s", error->message);
-          g_clear_error (&error);
-        }
-#else
-      arg = TRUE;
-
-      if (ioctlsocket (socket->priv->fd, FIONBIO, &arg) == SOCKET_ERROR)
-        {
-          int errsv = get_socket_errno ();
-          g_warning ("Error setting socket status flags: %s", socket_strerror (errsv));
-        }
-#endif
-
 #ifdef SO_NOSIGPIPE
       /* See note about SIGPIPE below. */
       g_socket_set_option (socket, SOL_SOCKET, SO_NOSIGPIPE, TRUE, NULL);
diff --git a/gio/gtestdbus.c b/gio/gtestdbus.c
index 34cead176..8c5de3355 100644
--- a/gio/gtestdbus.c
+++ b/gio/gtestdbus.c
@@ -249,7 +249,7 @@ watcher_init (void)
       gint pipe_fds[2];
 
       /* fork a child to clean up when we are killed */
-      if (!g_unix_open_pipe_internal (pipe_fds, TRUE))
+      if (!g_unix_open_pipe_internal (pipe_fds, TRUE, FALSE))
         {
           errsv = errno;
           g_warning ("pipe() failed: %s", g_strerror (errsv));
diff --git a/gio/inotify/inotify-kernel.c b/gio/inotify/inotify-kernel.c
index 92d61fc31..7733d398e 100644
--- a/gio/inotify/inotify-kernel.c
+++ b/gio/inotify/inotify-kernel.c
@@ -377,6 +377,7 @@ ik_source_new (gboolean (* callback) (ik_event_t *event))
   };
   InotifyKernelSource *iks;
   GSource *source;
+  gboolean should_set_nonblock = FALSE;
 
   source = g_source_new (&source_funcs, sizeof (InotifyKernelSource));
   iks = (InotifyKernelSource *) source;
@@ -384,17 +385,23 @@ ik_source_new (gboolean (* callback) (ik_event_t *event))
   g_source_set_static_name (source, "inotify kernel source");
 
   iks->unmatched_moves = g_hash_table_new (NULL, NULL);
-  iks->fd = inotify_init1 (IN_CLOEXEC);
+  iks->fd = inotify_init1 (IN_CLOEXEC | IN_NONBLOCK);
 
   if (iks->fd < 0)
-    iks->fd = inotify_init ();
+    {
+      should_set_nonblock = TRUE;
+      iks->fd = inotify_init ();
+    }
 
   if (iks->fd >= 0)
     {
       GError *error = NULL;
 
-      g_unix_set_fd_nonblocking (iks->fd, TRUE, &error);
-      g_assert_no_error (error);
+      if (should_set_nonblock)
+        {
+          g_unix_set_fd_nonblocking (iks->fd, TRUE, &error);
+          g_assert_no_error (error);
+        }
 
       iks->fd_tag = g_source_add_unix_fd (source, iks->fd, G_IO_IN);
     }
diff --git a/glib/gbacktrace.c b/glib/gbacktrace.c
index b708b1636..0f81502e5 100644
--- a/glib/gbacktrace.c
+++ b/glib/gbacktrace.c
@@ -398,8 +398,8 @@ stack_trace (const char * const *args)
   stack_trace_done = FALSE;
   signal (SIGCHLD, stack_trace_sigchld);
 
-  if (!g_unix_open_pipe_internal (in_fd, TRUE) ||
-      !g_unix_open_pipe_internal (out_fd, TRUE))
+  if (!g_unix_open_pipe_internal (in_fd, TRUE, FALSE) ||
+      !g_unix_open_pipe_internal (out_fd, TRUE, FALSE))
     {
       perror ("unable to open pipe");
       _exit (0);
diff --git a/glib/glib-unix.c b/glib/glib-unix.c
index 1c9f12599..ef0d1fbfb 100644
--- a/glib/glib-unix.c
+++ b/glib/glib-unix.c
@@ -74,11 +74,16 @@ g_unix_set_error_from_errno (GError **error,
  *
  * Similar to the UNIX pipe() call, but on modern systems like Linux
  * uses the pipe2() system call, which atomically creates a pipe with
- * the configured flags. The only supported flag currently is
- * %FD_CLOEXEC. If for example you want to configure %O_NONBLOCK, that
- * must still be done separately with fcntl().
+ * the configured flags.
  *
- * This function does not take %O_CLOEXEC, it takes %FD_CLOEXEC as if
+ * As of GLib 2.78, the supported flags are `FD_CLOEXEC` and `O_NONBLOCK`. Prior
+ * to GLib 2.78, only `FD_CLOEXEC` was supported — if you wanted to configure
+ * `O_NONBLOCK` then that had to be done separately with `fcntl()`.
+ *
+ * It is a programmer error to call this function with unsupported flags, and a
+ * critical warning will be raised.
+ *
+ * This function does not take `O_CLOEXEC`, it takes `FD_CLOEXEC` as if
  * for fcntl(); these are different on Linux/glibc.
  *
  * Returns: %TRUE on success, %FALSE if not (and errno will be set).
@@ -90,11 +95,12 @@ g_unix_open_pipe (int     *fds,
                   int      flags,
                   GError **error)
 {
-  /* We only support FD_CLOEXEC */
-  g_return_val_if_fail ((flags & (FD_CLOEXEC)) == flags, FALSE);
+  /* We only support FD_CLOEXEC and O_NONBLOCK */
+  g_return_val_if_fail ((flags & (FD_CLOEXEC | O_NONBLOCK)) == flags, FALSE);
 
   if (!g_unix_open_pipe_internal (fds,
-                                  (flags & FD_CLOEXEC) != 0))
+                                  (flags & FD_CLOEXEC) != 0,
+                                  (flags & O_NONBLOCK) != 0))
     return g_unix_set_error_from_errno (error, errno);
 
   return TRUE;
diff --git a/glib/glib-unixprivate.h b/glib/glib-unixprivate.h
index d4c64bd1e..fa13fe861 100644
--- a/glib/glib-unixprivate.h
+++ b/glib/glib-unixprivate.h
@@ -41,15 +41,22 @@ G_BEGIN_DECLS
 
 static inline gboolean
 g_unix_open_pipe_internal (int *fds,
-                           gboolean close_on_exec)
+                           gboolean close_on_exec,
+                           gboolean nonblock)
   {
 #ifdef HAVE_PIPE2
   do
     {
       int ecode;
+      int flags = 0;
+
+      if (close_on_exec)
+        flags |= O_CLOEXEC;
+      if (nonblock)
+        flags |= O_NONBLOCK;
 
       /* Atomic */
-      ecode = pipe2 (fds, close_on_exec ? O_CLOEXEC : 0);
+      ecode = pipe2 (fds, flags);
       if (ecode == -1 && errno != ENOSYS)
         return FALSE;
       else if (ecode == 0)
@@ -62,21 +69,44 @@ g_unix_open_pipe_internal (int *fds,
   if (pipe (fds) == -1)
     return FALSE;
 
-  if (!close_on_exec)
-    return TRUE;
+  if (close_on_exec)
+    {
+      if (fcntl (fds[0], F_SETFD, FD_CLOEXEC) == -1 ||
+          fcntl (fds[1], F_SETFD, FD_CLOEXEC) == -1)
+        {
+          int saved_errno = errno;
+
+          close (fds[0]);
+          close (fds[1]);
+          fds[0] = -1;
+          fds[1] = -1;
+
+          errno = saved_errno;
+          return FALSE;
+        }
+    }
 
-  if (fcntl (fds[0], F_SETFD, FD_CLOEXEC) == -1 ||
-      fcntl (fds[1], F_SETFD, FD_CLOEXEC) == -1)
+  if (nonblock)
     {
-      int saved_errno = errno;
+#ifdef O_NONBLOCK
+      int flags = O_NONBLOCK;
+#else
+      int flags = O_NDELAY;
+#endif
+
+      if (fcntl (fds[0], F_SETFL, flags) == -1 ||
+          fcntl (fds[1], F_SETFL, flags) == -1)
+        {
+          int saved_errno = errno;
 
-      close (fds[0]);
-      close (fds[1]);
-      fds[0] = -1;
-      fds[1] = -1;
+          close (fds[0]);
+          close (fds[1]);
+          fds[0] = -1;
+          fds[1] = -1;
 
-      errno = saved_errno;
-      return FALSE;
+          errno = saved_errno;
+          return FALSE;
+        }
     }
 
   return TRUE;
diff --git a/glib/gwakeup.c b/glib/gwakeup.c
index 24d85b669..a3283a3ff 100644
--- a/glib/gwakeup.c
+++ b/glib/gwakeup.c
@@ -160,7 +160,7 @@ g_wakeup_new (void)
   /* for any failure, try a pipe instead */
 #endif
 
-  if (!g_unix_open_pipe (wakeup->fds, FD_CLOEXEC, &error))
+  if (!g_unix_open_pipe (wakeup->fds, FD_CLOEXEC | O_NONBLOCK, &error))
     g_error ("Creating pipes for GWakeup: %s", error->message);
 
   if (!g_unix_set_fd_nonblocking (wakeup->fds[0], TRUE, &error) ||