Re: [Libguestfs] [nbdkit PATCH v2 07/17] build: Audit for use of pipe2
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 488 bytes)