[Libguestfs] [nbdkit PATCH v2 07/17] build: Audit for use of pipe2

[Eric Blake]

On 8/2/19 2:26 PM, Eric Blake wrote:
> Haiku unfortunately lacks pipe2, so we have to plan for a fallback at
> each site that uses it.  This also makes a good time to audit all
> existing users of pipe, to see if they should be using pipe2.  The
> tests fork() but don't fail because of fd leaks; and the nbd plugin
> doesn't fork() but was merely using pipe2 for convenience over
> multiple fcntl calls.  However, the server's quit_fd definitely needs
> to be marked CLOEXEC (it's easy to use the sh plugin to show that we
> are currently leaking it to children), although doing so can occur
> without worrying about atomicity since it is called before threads
> begin.  Finally, it's also worth updating our set_cloexec helper
> function to document that we still prefer atomicity where possible.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---

> +++ b/plugins/nbd/nbd.c
> @@ -431,11 +431,42 @@ nbdplug_open_handle (int readonly)
>      nbdkit_error ("malloc: %m");
>      return NULL;
>    }
> +#ifdef HAVE_PIPE2
>    if (pipe2 (h->fds, O_NONBLOCK)) {
> +    nbdkit_error ("pipe2: %m");
> +    free (h);
> +    return NULL;
> +  }
> +#else
> +  /* This plugin doesn't fork, so we don't care about CLOEXEC. Our use
> +   * of pipe2 is merely for convenience.
> +   */
> +  if (pipe (h->fds)) {
>      nbdkit_error ("pipe: %m");
>      free (h);
>      return NULL;
>    }
> +  else {
> +    int f;
> +
> +    f = fcntl (h->fds[0], F_GETFL);
> +    if (f == -1 || fcntl (h->fds[0], F_SETFL, f | O_NONBLOCK) == -1) {
> +      nbdkit_error ("fcntl: %m");

Shoot - I wrote up set_nonblock earlier in the series, then forgot to
use it here.  Consider this squashed in:

diff --git i/plugins/nbd/nbd.c w/plugins/nbd/nbd.c
index 95d910e7..f11e54d5 100644
--- i/plugins/nbd/nbd.c
+++ w/plugins/nbd/nbd.c
@@ -54,6 +54,7 @@
 #include <nbdkit-plugin.h>
 #include "byte-swapping.h"
 #include "cleanup.h"
+#include "utils.h"

 /* The per-transaction details */
 struct transaction {
@@ -446,25 +447,15 @@ nbdplug_open_handle (int readonly)
     free (h);
     return NULL;
   }
-  else {
-    int f;
-
-    f = fcntl (h->fds[0], F_GETFL);
-    if (f == -1 || fcntl (h->fds[0], F_SETFL, f | O_NONBLOCK) == -1) {
-      nbdkit_error ("fcntl: %m");
-      close (h->fds[0]);
-      close (h->fds[1]);
-      free (h);
-      return NULL;
-    }
-    f = fcntl (h->fds[1], F_GETFL);
-    if (f == -1 || fcntl (h->fds[1], F_SETFL, f | O_NONBLOCK) == -1) {
-      nbdkit_error ("fcntl: %m");
-      close (h->fds[0]);
-      close (h->fds[1]);
-      free (h);
-      return NULL;
-    }
+  if (set_nonblock (h->fds[0]) == -1) {
+    close (h->fds[1]);
+    free (h);
+    return NULL;
+  }
+  if (set_nonblock (h->fds[1]) == -1) {
+    close (h->fds[0]);
+    free (h);
+    return NULL;
   }
 #endif



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190802/bc7f4428/attachment.sig>