[Libguestfs] [PATCH] file: Better support for read-only files

[Richard W.M. Jones]

On Tue, Mar 02, 2021 at 07:21:06PM +0200, Nir Soffer wrote:
> On Tue, Mar 2, 2021 at 4:45 PM Eric Blake <eblake at redhat.com> wrote:
> >
> > Previously, attempts to use nbdkit without -r in order to visit a
> > read-only image failed to open the image (in fact, a single read-only
> > file in an otherwise usable directory makes "nbdkit file dir=. --run
> > 'nbdinfo --list'" fail to print anything in libnbd 1.6).  But a saner
> > approach is to try a fallback to a readonly fd if a read-write fd
> > fails.
> 
> So this is mainly a convenience if the user forgot -r, right?

It's allowed for a plugin to act as readonly even if the user doesn't
specify -r (example: nbdkit-pattern-plugin).  Or for plugins to defer
this decision until after the server has started up.  There is no
existing plugin (apart from nbdkit-file-plugin with Eric's patch)
which does that today, but you could imagine, say, an iscsi plugin
which would wire this up to the write_protect flag, which wouldn't be
known until the client connects.

This is controlled through the plugin's .can_write() method.  The only
way you can find out the final decision on this is using nbdinfo or
similar tools.

> > The windows code compiles, but I wasn't able to test it as thoroughly
> > as the Unix code.
> > ---
> >  plugins/file/file.c         | 30 ++++++++++++++++--
> >  plugins/file/winfile.c      | 21 +++++++++++--
> >  tests/Makefile.am           | 12 +++++--
> >  tests/test-file-readonly.sh | 63 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 118 insertions(+), 8 deletions(-)
> >  create mode 100755 tests/test-file-readonly.sh
> >
> > diff --git a/plugins/file/file.c b/plugins/file/file.c
> > index 1651079a..a9ecceb6 100644
> > --- a/plugins/file/file.c
> > +++ b/plugins/file/file.c
> > @@ -1,5 +1,5 @@
> >  /* nbdkit
> > - * Copyright (C) 2013-2020 Red Hat Inc.
> > + * Copyright (C) 2013-2021 Red Hat Inc.
> >   *
> >   * Redistribution and use in source and binary forms, with or without
> >   * modification, are permitted provided that the following conditions are
> > @@ -304,6 +304,7 @@ struct handle {
> >    int fd;
> >    bool is_block_device;
> >    int sector_size;
> > +  bool can_write;
> >    bool can_punch_hole;
> >    bool can_zero_range;
> >    bool can_fallocate;
> > @@ -343,12 +344,25 @@ file_open (int readonly)
> >    }
> >
> >    flags = O_CLOEXEC|O_NOCTTY;
> > -  if (readonly)
> > +  if (readonly) {
> >      flags |= O_RDONLY;
> > -  else
> > +    h->can_write = false;
> > +  }
> > +  else {
> >      flags |= O_RDWR;
> > +    h->can_write = true;
> > +  }
> >
> >    h->fd = openat (dfd, file, flags);
> > +  if (h->fd == -1 && !readonly) {
> > +    int saved_errno = errno;
> > +    flags = (flags & ~O_ACCMODE) | O_RDONLY;
> > +    h->fd = openat (dfd, file, flags);
> > +    if (h->fd == -1)
> > +      errno = saved_errno;
> > +    else
> > +      h->can_write = false;
> > +  }
> >    if (h->fd == -1) {
> >      nbdkit_error ("open: %s: %m", file);
> 
> This will always report the error that failed open when using O_RDWR.
> But if open(O_RDWR) fail with EPERM, and open(O_RDONLY) fails
> with another error, the other error will be hidden.
> 
> Maybe it is better to log the first error so we don't need to save errno
> when trying to open in read only mode?

Maybe we could put it in a nbdkit_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-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/