Libguestfs - Aug 2022 - [PATCH nbdkit 0/2] file: Add fd=<FILE_DESCRIPTOR>

Patch 1 looks better if you look at it with "git show -w".

This adds the ability to explicitly pass file descriptors to the file
plugin, matching a recent feature of qemu.  Actually this was already
possible on some platforms using file=/dev/fd/<N> but it seems better
to support it on all platforms[1], and make the feature and usage
explicit.

Rich.

[1] All platforms that have inheritable, numeric file descriptors, so
not Windows.

Previously we relied on the implicit assumption filename xor directory,
representing two modes.  Make this explicit with an internal mode
variable.

This is just refactoring and should not change the functionality.
However we're now more strict about duplicate file= or dir= parameters
appearing on the command line.  Previously only the last one had an
effect and the others were silently ignored.  Now we give an error in
this case.  eg this worked before but now gives an error:

  $ ./nbdkit file /var/tmp /var/tmp/fedora-36.img
  nbdkit: error: file|dir parameter can only appear once on the command line
---
 plugins/file/file.c | 124 +++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 48 deletions(-)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index aefca9ee2..f673ec132 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -70,6 +70,7 @@
 #include "isaligned.h"
 #include "fdatasync.h"
 
+static enum { mode_none, mode_filename, mode_directory } mode = mode_none;
 static char *filename = NULL;
 static char *directory = NULL;
 
@@ -180,14 +181,23 @@ file_config (const char *key, const char *value)
    * existence checks to the last possible moment.
    */
   if (strcmp (key, "file") == 0) {
-    free (filename);
+    if (mode != mode_none) {
+    wrong_mode:
+      nbdkit_error ("%s parameter can only appear once on the command
line",
+                    "file|dir");
+      return -1;
+    }
+    mode = mode_filename;
+    assert (filename == NULL);
     filename = nbdkit_realpath (value);
     if (!filename)
       return -1;
   }
   else if (strcmp (key, "directory") == 0 ||
            strcmp (key, "dir") == 0) {
-    free (directory);
+    if (mode != mode_none) goto wrong_mode;
+    mode = mode_directory;
+    assert (directory == NULL);
     directory = nbdkit_realpath (value);
     if (!directory)
       return -1;
@@ -252,22 +262,27 @@ file_config_complete (void)
   int r;
   struct stat sb;
 
-  if (!filename && !directory) {
+  if (mode == mode_none) {
     nbdkit_error ("you must supply either [file=]<FILENAME> or
"
                   "dir=<DIRNAME> parameter after the plugin name
"
                   "on the command line");
     return -1;
   }
-  if (filename && directory) {
-    nbdkit_error ("file= and dir= cannot be used at the same time");
-    return -1;
+  if (mode == mode_filename) {
+    assert (filename != NULL);
+    assert (directory == NULL);
+  }
+  if (mode == mode_directory) {
+    assert (filename == NULL);
+    assert (directory != NULL);
   }
 
   /* Sanity check now, rather than waiting for first client open.
    * See also comment in .config about use of nbdkit_realpath.
    * Yes, this is a harmless TOCTTOU race.
    */
-  if (filename) {
+  switch (mode) {
+  case mode_filename:
     r = stat (filename, &sb);
     if (r == 0 && S_ISDIR (sb.st_mode)) {
       nbdkit_error ("use dir= to serve files within %s", filename);
@@ -277,10 +292,14 @@ file_config_complete (void)
       nbdkit_error ("file is not regular or block device: %s",
filename);
       return -1;
     }
-  }
-  else if (stat (directory, &sb) == -1 || !S_ISDIR (sb.st_mode)) {
-    nbdkit_error ("expecting a directory: %s", directory);
-    return -1;
+    break;
+  case mode_directory:
+    if (stat (directory, &sb) == -1 || !S_ISDIR (sb.st_mode)) {
+      nbdkit_error ("expecting a directory: %s", directory);
+      return -1;
+    }
+    break;
+  default: abort ();
   }
 
   return 0;
@@ -320,46 +339,51 @@ file_list_exports (int readonly, int default_only,
   struct stat sb;
   int fd;
 
-  if (!directory)
+  switch (mode) {
+  case mode_filename:
     return nbdkit_add_export (exports, "", NULL);
 
-  dir = opendir (directory);
-  if (dir == NULL) {
-    nbdkit_error ("opendir: %m");
-    return -1;
-  }
-  fd = dirfd (dir);
-  if (fd == -1) {
-    nbdkit_error ("dirfd: %m");
-    closedir (dir);
-    return -1;
-  }
-  errno = 0;
-  while ((entry = readdir (dir)) != NULL) {
-    int r = -1;
-#if HAVE_STRUCT_DIRENT_D_TYPE
-    if (entry->d_type == DT_BLK || entry->d_type == DT_REG)
-      r = 1;
-    else if (entry->d_type != DT_LNK && entry->d_type !=
DT_UNKNOWN)
-      r = 0;
-#endif
-    /* TODO: when chasing symlinks, is statx any nicer than fstatat? */
-    if (r < 0 && fstatat (fd, entry->d_name, &sb, 0) == 0
&&
-        (S_ISREG (sb.st_mode) || S_ISBLK (sb.st_mode)))
-      r = 1;
-    if (r == 1 && nbdkit_add_export (exports, entry->d_name, NULL)
== -1) {
+  case mode_directory:
+    dir = opendir (directory);
+    if (dir == NULL) {
+      nbdkit_error ("opendir: %m");
+      return -1;
+    }
+    fd = dirfd (dir);
+    if (fd == -1) {
+      nbdkit_error ("dirfd: %m");
       closedir (dir);
       return -1;
     }
     errno = 0;
-  }
-  if (errno) {
-    nbdkit_error ("readdir: %m");
+    while ((entry = readdir (dir)) != NULL) {
+      int r = -1;
+#if HAVE_STRUCT_DIRENT_D_TYPE
+      if (entry->d_type == DT_BLK || entry->d_type == DT_REG)
+        r = 1;
+      else if (entry->d_type != DT_LNK && entry->d_type !=
DT_UNKNOWN)
+        r = 0;
+#endif
+      /* TODO: when chasing symlinks, is statx any nicer than fstatat? */
+      if (r < 0 && fstatat (fd, entry->d_name, &sb, 0) == 0
&&
+          (S_ISREG (sb.st_mode) || S_ISBLK (sb.st_mode)))
+        r = 1;
+      if (r == 1 && nbdkit_add_export (exports, entry->d_name, NULL)
== -1) {
+        closedir (dir);
+        return -1;
+      }
+      errno = 0;
+    }
+    if (errno) {
+      nbdkit_error ("readdir: %m");
+      closedir (dir);
+      return -1;
+    }
     closedir (dir);
-    return -1;
+    return 0;
+
+  default: abort ();
   }
-  closedir (dir);
-  return 0;
 }
 
 /* The per-connection handle. */
@@ -384,7 +408,8 @@ file_open (int readonly)
   const char *file;
   int dfd = -1;
 
-  if (directory) {
+  switch (mode) {
+  case mode_directory:
     file = nbdkit_export_name ();
     if (strchr (file, '/')) {
       nbdkit_error ("exportname cannot contain /");
@@ -396,9 +421,12 @@ file_open (int readonly)
       nbdkit_error ("open %s: %m", directory);
       return NULL;
     }
-  }
-  else
+    break;
+  case mode_filename:
     file = filename;
+    break;
+  default: abort ();
+  }
 
   h = malloc (sizeof *h);
   if (h == NULL) {
@@ -680,9 +708,9 @@ file_pwrite (void *handle, const void *buf, uint32_t count,
uint64_t offset,
 
 #if defined (FALLOC_FL_PUNCH_HOLE) || defined (FALLOC_FL_ZERO_RANGE)
 static int
-do_fallocate (int fd, int mode, off_t offset, off_t len)
+do_fallocate (int fd, int mode_, off_t offset, off_t len)
 {
-  int r = fallocate (fd, mode, offset, len);
+  int r = fallocate (fd, mode_, offset, len);
   if (r == -1 && errno == ENODEV) {
     /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails
        with EOPNOTSUPP in this case. Normalize errno to simplify callers. */
-- 
2.37.0.rc2

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