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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org