Libguestfs - Aug 2022 - [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin

This allows you to pass in a file descriptor to the plugin, eg:

  $ exec 6<>/var/tmp/fedora-36.img
  $ ./nbdkit file fd=6 --run 'nbdinfo $uri'

Note this was previously possible on most platforms using /dev/fd/<N>,
but not all platforms that have file descriptors support this
(although it is POSIX) and it seems better to make this explicit.
---
 plugins/file/nbdkit-file-plugin.pod |  22 +++++-
 tests/Makefile.am                   |   2 +
 plugins/file/file.c                 | 108 +++++++++++++++++++---------
 tests/test-file-fd.sh               |  65 +++++++++++++++++
 4 files changed, 160 insertions(+), 37 deletions(-)

diff --git a/plugins/file/nbdkit-file-plugin.pod
b/plugins/file/nbdkit-file-plugin.pod
index 49c330663..dadbe9432 100644
--- a/plugins/file/nbdkit-file-plugin.pod
+++ b/plugins/file/nbdkit-file-plugin.pod
@@ -11,6 +11,10 @@ nbdkit-file-plugin - nbdkit file plugin
 
  nbdkit file dir=DIRECTORY
 
+=for paragraph
+
+ nbdkit file fd=FILE_DESCRIPTOR
+
 =head1 DESCRIPTION
 
 C<nbdkit-file-plugin> is a file serving plugin for L<nbdkit(1)>.
@@ -22,10 +26,16 @@ If you use the C<dir> parameter the plugin works in a
different mode
 where it serves files from the given C<DIRECTORY>, chosen by the
 client using the NBD export name.
 
+If you use the C<fd> parameter then you can pass the file descriptor
+of a single disk to the plugin, inherited from the parent process.
+This can be useful where special permissions / capabilities are needed
+to open the file or you want to run nbdkit in a sandboxed environment.
+
 =head1 PARAMETERS
 
-Either B<file> or B<dir> must be given which controls the mode of
the
-plugin, either serving a single file or the files in a directory.
+B<file>, B<dir> or B<fd> must be given.  This controls the
mode of the
+plugin, either serving a single file, the files in a directory, or a
+single file descriptor.
 
 =over 4
 
@@ -72,6 +82,14 @@ read-ahead.  See also L<posix_fadvise(2)>.
 
 The default is C<normal>.
 
+=item B<fd=>FILE_DESCRIPTOR
+
+(nbdkit E<ge> 1.34, not Windows)
+
+The parameter is the number of a file descriptor.  Serve the file or
+device already open on this file descriptor.  The file descriptor is
+usually inherited from the parent process.
+
 =item [B<file=>]FILENAME
 
 Serve the file named C<FILENAME>.  A local block device name can also
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3dc428540..3667a1dea 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -732,10 +732,12 @@ EXTRA_DIST += \
 TESTS += \
 	test-file.sh \
 	test-file-readonly.sh \
+	test-file-fd.sh \
 	$(NULL)
 EXTRA_DIST += \
 	test-file.sh \
 	test-file-readonly.sh \
+	test-file-fd.sh \
 	$(NULL)
 LIBGUESTFS_TESTS += test-file-block
 
diff --git a/plugins/file/file.c b/plugins/file/file.c
index f673ec132..4e41e738a 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -70,9 +70,15 @@
 #include "isaligned.h"
 #include "fdatasync.h"
 
-static enum { mode_none, mode_filename, mode_directory } mode = mode_none;
+static enum {
+  mode_none,
+  mode_filename,
+  mode_directory,
+  mode_filedesc,
+} mode = mode_none;
 static char *filename = NULL;
 static char *directory = NULL;
+static int filedesc = -1;
 
 /* posix_fadvise mode: -1 = don't set it, or POSIX_FADV_*. */
 static int fadvise_mode @@ -184,7 +190,7 @@ file_config (const char *key, const
char *value)
     if (mode != mode_none) {
     wrong_mode:
       nbdkit_error ("%s parameter can only appear once on the command
line",
-                    "file|dir");
+                    "file|dir|fd");
       return -1;
     }
     mode = mode_filename;
@@ -202,6 +208,17 @@ file_config (const char *key, const char *value)
     if (!directory)
       return -1;
   }
+  else if (strcmp (key, "fd") == 0) {
+    if (mode != mode_none) goto wrong_mode;
+    mode = mode_filedesc;
+    assert (filedesc == -1);
+    if (nbdkit_parse_int ("fd", value, &filedesc) == -1)
+      return -1;
+    if (filedesc <= 2) {
+      nbdkit_error ("file descriptor must be >= 3");
+      return -1;
+    }
+  }
   else if (strcmp (key, "fadvise") == 0) {
     /* As this is a hint, if the kernel doesn't support the feature
      * ignore the parameter.
@@ -263,18 +280,26 @@ file_config_complete (void)
   struct stat sb;
 
   if (mode == mode_none) {
-    nbdkit_error ("you must supply either [file=]<FILENAME> or
"
-                  "dir=<DIRNAME> parameter after the plugin name
"
+    nbdkit_error ("you must supply [file=]<FILENAME>, "
+                  "dir=<DIRNAME> or fd=<FD> "
+                  "parameter after the plugin name "
                   "on the command line");
     return -1;
   }
   if (mode == mode_filename) {
     assert (filename != NULL);
     assert (directory == NULL);
+    assert (filedesc == -1);
   }
   if (mode == mode_directory) {
     assert (filename == NULL);
     assert (directory != NULL);
+    assert (filedesc == -1);
+  }
+  if (mode == mode_filedesc) {
+    assert (filename == NULL);
+    assert (directory == NULL);
+    assert (filedesc >= 3);
   }
 
   /* Sanity check now, rather than waiting for first client open.
@@ -299,6 +324,8 @@ file_config_complete (void)
       return -1;
     }
     break;
+  case mode_filedesc:
+    break;
   default: abort ();
   }
 
@@ -341,6 +368,7 @@ file_list_exports (int readonly, int default_only,
 
   switch (mode) {
   case mode_filename:
+  case mode_filedesc:
     return nbdkit_add_export (exports, "", NULL);
 
   case mode_directory:
@@ -408,59 +436,69 @@ file_open (int readonly)
   const char *file;
   int dfd = -1;
 
+  h = malloc (sizeof *h);
+  if (h == NULL) {
+    nbdkit_error ("malloc: %m");
+    return NULL;
+  }
+  h->can_write = !readonly;
+  h->fd = -1;
+
   switch (mode) {
   case mode_directory:
     file = nbdkit_export_name ();
     if (strchr (file, '/')) {
       nbdkit_error ("exportname cannot contain /");
+      free (h);
       errno = EINVAL;
       return NULL;
     }
     dfd = open (directory, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
     if (dfd == -1) {
       nbdkit_error ("open %s: %m", directory);
+      free (h);
       return NULL;
     }
     break;
   case mode_filename:
     file = filename;
     break;
+  case mode_filedesc:
+    h->fd = dup (filedesc);
+    if (h->fd == -1) {
+      nbdkit_error ("dup fd=%d: %m", filedesc);
+      free (h);
+      return NULL;
+    }
+    file = "<file descriptor>"; /* This is needed for error
messages. */
+    break;
   default: abort ();
   }
 
-  h = malloc (sizeof *h);
-  if (h == NULL) {
-    nbdkit_error ("malloc: %m");
-    if (dfd != -1)
-      close (dfd);
-    return NULL;
-  }
-
-  flags = O_CLOEXEC|O_NOCTTY;
-  if (readonly) {
-    flags |= O_RDONLY;
-    h->can_write = false;
-  }
-  else {
-    flags |= O_RDWR;
-    h->can_write = true;
-  }
-
-  h->fd = openat (dfd, file, flags);
-  if (h->fd == -1 && !readonly) {
-    nbdkit_debug ("open O_RDWR failed, falling back to read-only: %s:
%m",
-                  file);
-    flags = (flags & ~O_ACCMODE) | O_RDONLY;
-    h->fd = openat (dfd, file, flags);
-    h->can_write = false;
-  }
   if (h->fd == -1) {
-    nbdkit_error ("open: %s: %m", file);
-    if (dfd != -1)
-      close (dfd);
-    free (h);
-    return NULL;
+    flags = O_CLOEXEC|O_NOCTTY;
+    if (readonly)
+      flags |= O_RDONLY;
+    else
+      flags |= O_RDWR;
+
+    h->fd = openat (dfd, file, flags);
+    if (h->fd == -1 && !readonly) {
+      nbdkit_debug ("open O_RDWR failed, falling back to read-only: %s:
%m",
+                    file);
+      flags = (flags & ~O_ACCMODE) | O_RDONLY;
+      h->fd = openat (dfd, file, flags);
+      h->can_write = false;
+    }
+    if (h->fd == -1) {
+      nbdkit_error ("open: %s: %m", file);
+      if (dfd != -1)
+        close (dfd);
+      free (h);
+      return NULL;
+    }
   }
+
   if (dfd != -1)
     close (dfd);
 
diff --git a/tests/test-file-fd.sh b/tests/test-file-fd.sh
new file mode 100755
index 000000000..6c98ad9e5
--- /dev/null
+++ b/tests/test-file-fd.sh
@@ -0,0 +1,65 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2013-2022 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the file plugin fd parameter.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin file
+requires_nbdsh_uri
+requires $TRUNCATE --version
+
+sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
+files="file-fd.pid file-fd.img $sock"
+rm -f $files
+cleanup_fn rm -f $files
+
+$TRUNCATE -s 16384 file-fd.img
+exec 6<>file-fd.img
+start_nbdkit -P file-fd.pid -U $sock file fd=6
+
+nbdsh -u "nbd+unix://?socket=$sock" -c '
+assert not h.is_read_only()
+assert h.get_size() == 16384
+
+buf0 = bytearray(1024)
+buf1 = b"1" * 1024
+buf2 = b"2" * 1024
+h.pwrite(buf1 + buf2 + buf1 + buf2, 1024)
+buf = h.pread(8192, 0)
+assert buf == buf0 + buf1 + buf2 + buf1 + buf2 + buf0*3
+h.zero(4096, 1024)
+buf = h.pread(8192, 0)
+assert buf == buf0*8
+'
-- 
2.37.0.rc2

On Wed, Aug 17, 2022 at 04:38:11PM +0100, Richard W.M. Jones
wrote:
> This allows you to pass in a file descriptor to the plugin, eg:
> 
>   $ exec 6<>/var/tmp/fedora-36.img
>   $ ./nbdkit file fd=6 --run 'nbdinfo $uri'
> 
> Note this was previously possible on most platforms using
/dev/fd/<N>,
> but not all platforms that have file descriptors support this
> (although it is POSIX) and it seems better to make this explicit.
The parenthetical feels odd here.  /dev/fd/<N> is not POSIX; what IS a
POSIX feature is the ability to pass an open file descriptor by
inheritance.
> @@ -202,6 +208,17 @@ file_config (const char *key, const char *value)
>      if (!directory)
>        return -1;
>    }
> +  else if (strcmp (key, "fd") == 0) {
> +    if (mode != mode_none) goto wrong_mode;
> +    mode = mode_filedesc;
> +    assert (filedesc == -1);
> +    if (nbdkit_parse_int ("fd", value, &filedesc) == -1)
> +      return -1;
> +    if (filedesc <= 2) {
> +      nbdkit_error ("file descriptor must be >= 3");
Is it worth using STDERR_FILENO instead of open-coding 2? I'll be okay
if your answer is that this is one place where <unistd.h> has too much
indirection ;)
> @@ -408,59 +436,69 @@ file_open (int readonly)
...
> +  case mode_filedesc:
> +    h->fd = dup (filedesc);
> +    if (h->fd == -1) {
> +      nbdkit_error ("dup fd=%d: %m", filedesc);
> +      free (h);
> +      return NULL;
> +    }
> +    file = "<file descriptor>"; /* This is needed for
error messages. */
> +    break;
Is it worth trying to figure out if the FD has O_RDWR privileges and
set h->can_write accordingly, to match...
>    default: abort ();
>    }
>  
> -  h = malloc (sizeof *h);
> -  if (h == NULL) {
> -    nbdkit_error ("malloc: %m");
> -    if (dfd != -1)
> -      close (dfd);
> -    return NULL;
> -  }
> -
> -  flags = O_CLOEXEC|O_NOCTTY;
> -  if (readonly) {
> -    flags |= O_RDONLY;
> -    h->can_write = false;
> -  }
> -  else {
> -    flags |= O_RDWR;
> -    h->can_write = true;
> -  }
> -
> -  h->fd = openat (dfd, file, flags);
> -  if (h->fd == -1 && !readonly) {
> -    nbdkit_debug ("open O_RDWR failed, falling back to read-only: %s:
%m",
> -                  file);
> -    flags = (flags & ~O_ACCMODE) | O_RDONLY;
> -    h->fd = openat (dfd, file, flags);
> -    h->can_write = false;
> -  }
... what we do for normal files?

Overall looks like a nice addition.

ACK series

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