Libguestfs - Aug 2022 - [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/<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

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/<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.
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, &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 ;)
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 = "<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...
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