Libguestfs - Mar 2017 - [PATCH 1/2] Use gnulib set_nonblocking_flag function instead of fcntl.

The previous code:

  fcntl (fd, F_SETFL, O_NONBLOCK)

was technically incorrect, because it would have reset any
other flags on the file descriptor.

Thanks: Eric Blake
---
 bootstrap         |  1 +
 daemon/inotify.c  |  6 ++++--
 lib/conn-socket.c | 21 +++++++++++----------
 m4/.gitignore     |  9 +++++++++
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/bootstrap b/bootstrap
index faa10a3..77a95a2 100755
--- a/bootstrap
+++ b/bootstrap
@@ -75,6 +75,7 @@ mkdtemp
 mkstemps
 netdb
 netinet_in
+nonblocking
 openat
 perror
 pipe2
diff --git a/daemon/inotify.c b/daemon/inotify.c
index 4360866..93722d0 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -29,6 +29,8 @@
 #include <sys/inotify.h>
 #endif
 
+#include "nonblocking.h"
+
 #include "guestfs_protocol.h"
 #include "daemon.h"
 #include "actions.h"
@@ -112,8 +114,8 @@ do_inotify_init (int max_events)
     reply_with_perror ("inotify_init");
     return -1;
   }
-  if (fcntl (inotify_fd, F_SETFL, O_NONBLOCK) == -1) {
-    reply_with_perror ("fcntl: O_NONBLOCK");
+  if (set_nonblocking_flag (inotify_fd, 1) == -1) {
+    reply_with_perror ("set_nonblocking_flag");
     close (inotify_fd);
     inotify_fd = -1;
     return -1;
diff --git a/lib/conn-socket.c b/lib/conn-socket.c
index 4e1f781..2cd261a 100644
--- a/lib/conn-socket.c
+++ b/lib/conn-socket.c
@@ -37,6 +37,7 @@
 #include <libintl.h>
 
 #include "ignore-value.h"
+#include "nonblocking.h"
 
 #include "guestfs.h"
 #include "guestfs-internal.h"
@@ -129,8 +130,8 @@ accept_connection (guestfs_h *g, struct connection *connv)
   conn->daemon_sock = sock;
 
   /* Make sure the new socket is non-blocking. */
-  if (fcntl (conn->daemon_sock, F_SETFL, O_NONBLOCK) == -1) {
-    perrorf (g, "accept_connection: fcntl");
+  if (set_nonblocking_flag (conn->daemon_sock, 1) == -1) {
+    perrorf (g, "accept_connection: set_nonblocking_flag");
     return -1;
   }
 
@@ -438,14 +439,14 @@ guestfs_int_new_conn_socket_listening (guestfs_h *g,
 
   assert (daemon_accept_sock >= 0);
 
-  if (fcntl (daemon_accept_sock, F_SETFL, O_NONBLOCK) == -1) {
-    perrorf (g, "new_conn_socket_listening: fcntl");
+  if (set_nonblocking_flag (daemon_accept_sock, 1) == -1) {
+    perrorf (g, "new_conn_socket_listening: set_nonblocking_flag");
     return NULL;
   }
 
   if (console_sock >= 0) {
-    if (fcntl (console_sock, F_SETFL, O_NONBLOCK) == -1) {
-      perrorf (g, "new_conn_socket_listening: fcntl");
+    if (set_nonblocking_flag (console_sock, 1) == -1) {
+      perrorf (g, "new_conn_socket_listening: set_nonblocking_flag");
       return NULL;
     }
   }
@@ -478,14 +479,14 @@ guestfs_int_new_conn_socket_connected (guestfs_h *g,
 
   assert (daemon_sock >= 0);
 
-  if (fcntl (daemon_sock, F_SETFL, O_NONBLOCK) == -1) {
-    perrorf (g, "new_conn_socket_connected: fcntl");
+  if (set_nonblocking_flag (daemon_sock, 1) == -1) {
+    perrorf (g, "new_conn_socket_connected: set_nonblocking_flag");
     return NULL;
   }
 
   if (console_sock >= 0) {
-    if (fcntl (console_sock, F_SETFL, O_NONBLOCK) == -1) {
-      perrorf (g, "new_conn_socket_connected: fcntl");
+    if (set_nonblocking_flag (console_sock, 1) == -1) {
+      perrorf (g, "new_conn_socket_connected: set_nonblocking_flag");
       return NULL;
     }
   }
diff --git a/m4/.gitignore b/m4/.gitignore
index bbe7a9d..009cd5b 100644
--- a/m4/.gitignore
+++ b/m4/.gitignore
@@ -41,6 +41,7 @@
 /exponentd.m4
 /extensions.m4
 /extern-inline.m4
+/fatal-signal.m4
 /fchdir.m4
 /fclose.m4
 /fcntl_h.m4
@@ -156,6 +157,7 @@
 /netdb_h.m4
 /netinet_in_h.m4
 /nocrash.m4
+/nonblocking.m4
 /off_t.m4
 /onceonly.m4
 /openat.m4
@@ -165,6 +167,7 @@
 /perror.m4
 /pipe2.m4
 /pipe.m4
+/posix_spawn.m4
 /pread.m4
 /printf.m4
 /priv-set.m4
@@ -186,12 +189,14 @@
 /safe-read.m4
 /safe-write.m4
 /save-cwd.m4
+/sched_h.m4
 /secure_getenv.m4
 /select.m4
 /servent.m4
 /setenv.m4
 /setlocale.m4
 /sigaction.m4
+/sig_atomic_t.m4
 /signalblocking.m4
 /signal_h.m4
 /signed.m4
@@ -202,6 +207,7 @@
 /sockets.m4
 /socklen.m4
 /sockpfaf.m4
+/spawn_h.m4
 /ssize_t.m4
 /stat.m4
 /stat-time.m4
@@ -256,6 +262,9 @@
 /utimes.m4
 /vasnprintf.m4
 /vasprintf.m4
+/vsnprintf.m4
+/waitpid.m4
+/wait-process.m4
 /warnings.m4
 /warn-on-use.m4
 /wchar_h.m4
-- 
2.9.3

---
 daemon/inotify.c               |  5 +++--
 fuse/test-guestunmount-fd.c    |  3 ++-
 tests/c-api/test-user-cancel.c | 14 ++++++++------
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/daemon/inotify.c b/daemon/inotify.c
index 93722d0..38a1c92 100644
--- a/daemon/inotify.c
+++ b/daemon/inotify.c
@@ -29,6 +29,7 @@
 #include <sys/inotify.h>
 #endif
 
+#include "cloexec.h"
 #include "nonblocking.h"
 
 #include "guestfs_protocol.h"
@@ -120,8 +121,8 @@ do_inotify_init (int max_events)
     inotify_fd = -1;
     return -1;
   }
-  if (fcntl (inotify_fd, F_SETFD, FD_CLOEXEC) == -1) {
-    reply_with_perror ("fcntl: FD_CLOEXEC");
+  if (set_cloexec_flag (inotify_fd, 1) == -1) {
+    reply_with_perror ("set_cloexec_flag");
     close (inotify_fd);
     inotify_fd = -1;
     return -1;
diff --git a/fuse/test-guestunmount-fd.c b/fuse/test-guestunmount-fd.c
index 019f821..6756f18 100644
--- a/fuse/test-guestunmount-fd.c
+++ b/fuse/test-guestunmount-fd.c
@@ -32,6 +32,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 
+#include "cloexec.h"
 #include "ignore-value.h"
 
 #include "guestfs.h"
@@ -77,7 +78,7 @@ main (int argc, char *argv[])
 
   /* Parent continues. */
   close (pipefd[0]);
-  ignore_value (fcntl (pipefd[1], F_SETFD, FD_CLOEXEC));
+  ignore_value (set_cloexec_flag (pipefd[1], 1));
 
   /* Sleep a bit and test that the guestunmount process is still running. */
   sleep (2);
diff --git a/tests/c-api/test-user-cancel.c b/tests/c-api/test-user-cancel.c
index 3823682..d429a0f 100644
--- a/tests/c-api/test-user-cancel.c
+++ b/tests/c-api/test-user-cancel.c
@@ -45,6 +45,8 @@
 
 #include <pthread.h>
 
+#include "cloexec.h"
+
 #include "guestfs.h"
 #include "guestfs-internal-frontend.h"
 
@@ -102,9 +104,9 @@ main (int argc, char *argv[])
     error (EXIT_FAILURE, errno, "pipe");
 
   /* We don't want the pipe to be passed to subprocesses. */
-  if (fcntl (fds[0], F_SETFD, FD_CLOEXEC) == -1 ||
-      fcntl (fds[1], F_SETFD, FD_CLOEXEC) == -1)
-    error (EXIT_FAILURE, errno, "fcntl");
+  if (set_cloexec_flag (fds[0], 1) == -1 ||
+      set_cloexec_flag (fds[1], 1) == -1)
+    error (EXIT_FAILURE, errno, "set_cloexec_flag");
 
   data.fd = fds[1];
   snprintf (dev_fd, sizeof dev_fd, "/dev/fd/%d", fds[0]);
@@ -160,9 +162,9 @@ main (int argc, char *argv[])
     error (EXIT_FAILURE, errno, "pipe");
 
   /* We don't want the pipe to be passed to subprocesses. */
-  if (fcntl (fds[0], F_SETFD, FD_CLOEXEC) == -1 ||
-      fcntl (fds[1], F_SETFD, FD_CLOEXEC) == -1)
-    error (EXIT_FAILURE, errno, "fcntl");
+  if (set_cloexec_flag (fds[0], 1) == -1 ||
+      set_cloexec_flag (fds[1], 1) == -1)
+    error (EXIT_FAILURE, errno, "set_cloexec_flag");
 
   data.fd = fds[0];
   snprintf (dev_fd, sizeof dev_fd, "/dev/fd/%d", fds[1]);
-- 
2.9.3

On Friday, 3 March 2017 16:00:22 CET Richard W.M. Jones
wrote:
> The previous code:
> 
>   fcntl (fd, F_SETFL, O_NONBLOCK)
> 
> was technically incorrect, because it would have reset any
> other flags on the file descriptor.
> 
> Thanks: Eric Blake
> ---
The series LGTM.

As additional work for NONBLOCK, I'd use set_nonblocking_flag instead
of SOCK_NONBLOCK in the only usage of it in accept4, i.e.
lib/launch-libvirt.c, so we can get rid of one of the (IMHO) wrong
definitions for non-Linux added by commit
7ddf6bcbfdc66855b594afaaacdc4998177f2286.

Thanks,
-- 
Pino Toscano