Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On Thu, Aug 18, 2022 at 04:36:13PM +0100, Richard W.M. Jones wrote: > This is what I pushed - how does it look? I ignored the cases we > cannot deal with, except O_WRONLY where I emit a debug message but > continue: > > https://gitlab.com/nbdkit/nbdkit/-/blob/master/plugins/file/file.c#L572 > > > > > > > There's also the case where r == O_WRONLY which the plugin (and NBD) > > > cannot deal with. Not sure what to do about that - error? > > > > Or allow it, but with the caveat that every NBD_CMD_READ will fail. > > The only reason to special case h->can_write=false for O_RDONLY is > > because then we don't advertise it to the client; to save them from > > getting failures on NBD_CMD_WRITE - but that's because it is an easy > > thing to advertise. Advertising that NBD_CMD_READ will fail is not > > easy (and less likely to happen in practice), so failing to serve the > > file is just as viable as serving it and letting every NBD_CMD_READ > > fail. > > That's basically what I did, plus a debug message :-) What you have committed looks okay to me for handling the common case of O_RDONLY without worrying about the other cases. And if someone tries to play with passing in an fd opened under O_EXEC, they deserve whatever behavior happens naturally from that explicitly unusual corner-case. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On Thu, Aug 18, 2022 at 10:28:17AM -0500, Eric Blake wrote:
> On Wed, Aug 17, 2022 at 10:56:58PM +0100, Richard W.M. Jones wrote:
> > On Wed, Aug 17, 2022 at 10:37:00PM +0100, Richard W.M. Jones wrote:
> > > Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY”
> > > should do it?
> >
> > So the answer is no as it's a kind of tri-state.
>
> More like 5-state (by POSIX): O_RDONLY, O_WRONLY, O_RDWR, O_SEARCH,
> O_EXEC (although O_SEARCH and O_EXEC can be the same bit, because
> their uses are mutually exclusive). And in reality, since Linux burns
> 3 bits on it, there can be other states as well (O_PATH, for example).
>
> Some systems have O_RDONLY==1, O_WRONLY==2, O_RDWR==3 (then an obvious
> extension for O_PATH would be 0); but most systems have O_RDONLY==0,
> O_WRONLY==1, O_RDWR==2, which makes bit-wise testing impossible.
>
> >
> > I think this should work (untested)?
> >
> > r = fcntl (fd, F_GETFL);
> > if (r == -1) ...
> > r &= O_ACCMODE;
> > if (r == O_RDONLY)
> > h->can_write = false;
>
> Yeah, that's one approach. Another might be as Lazslo suggested:
>
> switch (r & O_ACCMODE) {
> case O_RDWR: break;
> case O_RDONLY: h->can_write = false; break;
> default: //error about unsupported mode
> }
This is what I pushed - how does it look? I ignored the cases we
cannot deal with, except O_WRONLY where I emit a debug message but
continue:
https://gitlab.com/nbdkit/nbdkit/-/blob/master/plugins/file/file.c#L572
> >
> > There's also the case where r == O_WRONLY which the plugin (and NBD)
> > cannot deal with. Not sure what to do about that - error?
>
> Or allow it, but with the caveat that every NBD_CMD_READ will fail.
> The only reason to special case h->can_write=false for O_RDONLY is
> because then we don't advertise it to the client; to save them from
> getting failures on NBD_CMD_WRITE - but that's because it is an easy
> thing to advertise. Advertising that NBD_CMD_READ will fail is not
> easy (and less likely to happen in practice), so failing to serve the
> file is just as viable as serving it and letting every NBD_CMD_READ
> fail.
That's basically what I did, plus a debug message :-)
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On Wed, Aug 17, 2022 at 10:56:58PM +0100, Richard W.M. Jones wrote:
> On Wed, Aug 17, 2022 at 10:37:00PM +0100, Richard W.M. Jones wrote:
> > Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY”
> > should do it?
>
> So the answer is no as it's a kind of tri-state.
More like 5-state (by POSIX): O_RDONLY, O_WRONLY, O_RDWR, O_SEARCH,
O_EXEC (although O_SEARCH and O_EXEC can be the same bit, because
their uses are mutually exclusive). And in reality, since Linux burns
3 bits on it, there can be other states as well (O_PATH, for example).
Some systems have O_RDONLY==1, O_WRONLY==2, O_RDWR==3 (then an obvious
extension for O_PATH would be 0); but most systems have O_RDONLY==0,
O_WRONLY==1, O_RDWR==2, which makes bit-wise testing impossible.
>
> I think this should work (untested)?
>
> r = fcntl (fd, F_GETFL);
> if (r == -1) ...
> r &= O_ACCMODE;
> if (r == O_RDONLY)
> h->can_write = false;
Yeah, that's one approach. Another might be as Lazslo suggested:
switch (r & O_ACCMODE) {
case O_RDWR: break;
case O_RDONLY: h->can_write = false; break;
default: //error about unsupported mode
}
>
> There's also the case where r == O_WRONLY which the plugin (and NBD)
> cannot deal with. Not sure what to do about that - error?
Or allow it, but with the caveat that every NBD_CMD_READ will fail.
The only reason to special case h->can_write=false for O_RDONLY is
because then we don't advertise it to the client; to save them from
getting failures on NBD_CMD_WRITE - but that's because it is an easy
thing to advertise. Advertising that NBD_CMD_READ will fail is not
easy (and less likely to happen in practice), so failing to serve the
file is just as viable as serving it and letting every NBD_CMD_READ
fail.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On 08/17/22 23:56, Richard W.M. Jones wrote: > On Wed, Aug 17, 2022 at 10:37:00PM +0100, Richard W.M. Jones wrote: >> Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY” >> should do it? > > So the answer is no as it's a kind of tri-state. > > I think this should work (untested)? > > r = fcntl (fd, F_GETFL); > if (r == -1) ... > r &= O_ACCMODE; > if (r == O_RDONLY) > h->can_write = false; > > There's also the case where r == O_WRONLY which the plugin (and NBD) > cannot deal with. Not sure what to do about that - error? (after a bit of reading POSIX): I think a switch statement should handle these; O_RDONLY and O_RDWR intuitively, everything else (O_EXEC, O_SEARCH, O_WRONLY) should be rejected. Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On Wed, Aug 17, 2022 at 10:37:00PM +0100, Richard W.M. Jones wrote: > Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY” > should do it? So the answer is no as it's a kind of tri-state. I think this should work (untested)? r = fcntl (fd, F_GETFL); if (r == -1) ... r &= O_ACCMODE; if (r == O_RDONLY) h->can_write = false; There's also the case where r == O_WRONLY which the plugin (and NBD) cannot deal with. Not sure what to do about that - error? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
On Wed, Aug 17, 2022 at 03:57:24PM -0500, Eric Blake wrote:
> 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/,
> > 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/ is not POSIX; what IS a
> POSIX feature is the ability to pass an open file descriptor by
> inheritance.
Will fix.
> > @@ -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, ) == -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 has too much
> indirection ;)
Will fix.
> > @@ -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 = ""; /* 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...
Is that actually possible? “fcntl (fd, F_GETFL) & O_WRONLY”
should do it?
> >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
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 2/2] file: Allow a file descriptor to be passed to the plugin
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/,
> 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/ 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, ) == -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 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 = ""; /* 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [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/,
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 is a file serving plugin for L.
@@ -22,10 +26,16 @@ If you use the C parameter the plugin works in a
different mode
where it serves files from the given C, chosen by the
client using the NBD export name.
+If you use the C 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 or B must be given which controls the mode of the
-plugin, either serving a single file or the files in a directory.
+B, B or B 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.
The default is C.
+=item BFILE_DESCRIPTOR
+
+(nbdkit E 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]FILENAME
Serve the file named C. 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, ) == -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=] or "
- "dir= parameter after the plugin name "
+nbdkit_error ("you must supply [file=], "
+ "dir= or 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 @@
