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

On Thu, Aug 18, 2022 at 10:27:11AM +0200, Laszlo Ersek wrote: I don't think I really have a problem with it, but I moved it to the end of the function. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On Thu, Aug 18, 2022 at 10:27:11AM +0200, Laszlo Ersek wrote: I've used the same style in recent patches to libnbd (because it was pre-existing), but I can also agree to favor a forward jump to a common error handling at the bottom of the function rather than a backward jump to a mid-if label. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

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 03:57:24PM -0500, Eric Blake wrote: Will fix. Will fix. Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY” should do it? I also want to add dirfd= similarly for directory FDs (seems better to be explicit rather than overloading fd= and distinguishing through stat?) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html