<div dir="ltr"><div dir="ltr">On Tue, Mar 2, 2021 at 8:25 PM Eric Blake <<a href="mailto:eblake@redhat.com">eblake@redhat.com</a>> wrote: </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/2/21 11:21 AM, Nir Soffer wrote: > On Tue, Mar 2, 2021 at 4:45 PM Eric Blake <<a href="mailto:eblake@redhat.com" target="_blank">eblake@redhat.com</a>> 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 <a href="http://podwrapper.pl" rel="noreferrer" target="_blank">podwrapper.pl</a> in that directory from the <a href="http://podwrapper.pl.in" rel="noreferrer" target="_blank">podwrapper.pl.in</a> 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. </blockquote><div> </div><div>Agree, it looks useful, and simplifies usage.</div><div> </div></div></div>