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

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