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

[Nir Soffer]

On Tue, Mar 2, 2021 at 8:25 PM Eric Blake <eblake at redhat.com> wrote:

> On 3/2/21 11:21 AM, 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?
>
> Not just that.  Right now, the NBD protocol has no way for the client to
> advertise its intent to the server.  I'd love to add an extension where
> a client can hint that it intends to be read-only, or that it wants
> write access, to allow a server to further fine-tune which files it
> advertises as available.  But without that extension, the choice of
> whether to be read-only resides solely on the server, prior to the
> client even being started.  Advertising something, then being unable to
> expose it after all, is annoying.
>
> Perhaps we could _also_ patch file dir=... mode to not even advertise
> read-only files if nbdkit was started without -r, but the graceful
> fallback to readonly seems nicer than requiring the user to remember to
> start nbdkit with -r.
>
> And for reference, I hit it while doing:
>
> nbdkit file dir=path/to/nbdkit --run 'nbdinfo --list'
>
> which failed to show anything at all (until my companion patch to libnbd
> earlier today, which I already pushed as it was not as challenging as
> this one), all because the nbdkit build process creates a readonly
> podwrapper.pl in that directory from the podwrapper.pl.in file.
>
> >>    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.
>
> Generally, the first error is the one that matters most.  POSIX says
> that when more than one error condition applies, it is up to the
> implementation which one it wants to return.  But in practice, something
> like ENOENT will take precedence, and both the O_RDWR and O_RDONLY
> attempts will fail with the same ENOENT, and not the O_RDWR failing due
> to EPERM.
>
> >
> > 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?
>
> I didn't see it as that much added value.  I also considered checking
> for specific values of errno (EPERM, EROFS, others?) but figured I'd
> probably miss something.  I also tried to avoid the TOCTTOU race of
> checking stat() before open() (avoiding a second open if the stat says
> no read permissions, but then being potentially stale data if the file
> changed between the two calls).
>
> >
> > This can make it easier to use, but on the other hand it complicates
> > the code and error handling when a user could use --readdonly, so
> > I'm not sure about this.
>
> Requiring the server user to pass -r before even starting nbdkit is not
> as nice as if the client could request -r, but that NBD extension
> doesn't exist yet.
>

Agree, it looks useful, and simplifies usage.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210302/7c6e6265/attachment.htm>