Nir Soffer
2021-Mar-02 17:21 UTC
[Libguestfs] [PATCH] file: Better support for read-only files
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?> 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?> if (dfd != -1) > @@ -464,6 +478,15 @@ file_get_size (void *handle) > } > } > > +/* Check if file is read-only. */ > +static int > +file_can_write (void *handle) > +{ > + struct handle *h = handle; > + > + return h->can_write; > +} > + > /* Allow multiple parallel connections from a single client. */ > static int > file_can_multi_conn (void *handle) > @@ -880,6 +903,7 @@ static struct nbdkit_plugin plugin = { > .open = file_open, > .close = file_close, > .get_size = file_get_size, > + .can_write = file_can_write, > .can_multi_conn = file_can_multi_conn, > .can_trim = file_can_trim, > .can_fua = file_can_fua, > diff --git a/plugins/file/winfile.c b/plugins/file/winfile.c > index 6d1a6643..ec186255 100644 > --- a/plugins/file/winfile.c > +++ b/plugins/file/winfile.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 > @@ -106,6 +106,7 @@ winfile_dump_plugin (void) > struct handle { > HANDLE fh; > int64_t size; > + bool is_readonly; > bool is_volume; > bool is_sparse; > }; > @@ -126,6 +127,12 @@ winfile_open (int readonly) > > fh = CreateFile (filename, flags, FILE_SHARE_READ|FILE_SHARE_WRITE, > NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); > + if (fh == INVALID_HANDLE_VALUE && !readonly) { > + flags &= ~GENERIC_WRITE; > + readonly = true; > + fh = CreateFile (filename, flags, FILE_SHARE_READ|FILE_SHARE_WRITE, > + NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); > + } > if (fh == INVALID_HANDLE_VALUE) { > nbdkit_error ("%s: error %lu", filename, GetLastError ()); > return NULL; > @@ -179,15 +186,24 @@ winfile_open (int readonly) > } > h->fh = fh; > h->size = size.QuadPart; > + h->is_readonly = readonly; > h->is_volume = is_volume; > h->is_sparse = is_sparse; > - nbdkit_debug ("%s: size=%" PRIi64 " is_volume=%s is_sparse=%s", > + nbdkit_debug ("%s: size=%" PRIi64 " readonly=%s is_volume=%s is_sparse=%s", > filename, h->size, > + readonly ? "true" : "false", > is_volume ? "true" : "false", > is_sparse ? "true" : "false"); > return h; > } > > +static int > +wintfile_can_write (void *handle) > +{ > + struct handle *h = handle; > + return !h->is_readonly; > +} > + > static int > winfile_can_trim (void *handle) > { > @@ -425,6 +441,7 @@ static struct nbdkit_plugin plugin = { > .dump_plugin = winfile_dump_plugin, > > .open = winfile_open, > + .can_write = winfile_can_write, > .can_trim = winfile_can_trim, > .can_zero = winfile_can_zero, > .can_extents = winfile_can_extents, > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 70898f20..e8c9c535 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -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 > @@ -673,8 +673,14 @@ EXTRA_DIST += \ > $(NULL) > > # file plugin test. > -TESTS += test-file.sh > -EXTRA_DIST += test-file.sh > +TESTS += \ > + test-file.sh \ > + test-file-readonly.sh \ > + $(NULL) > +EXTRA_DIST += \ > + test-file.sh \ > + test-file-readonly.sh \ > + $(NULL) > LIBGUESTFS_TESTS += test-file-block > > test_file_block_SOURCES = test-file-block.c test.h > diff --git a/tests/test-file-readonly.sh b/tests/test-file-readonly.sh > new file mode 100755 > index 00000000..cde2bd65 > --- /dev/null > +++ b/tests/test-file-readonly.sh > @@ -0,0 +1,63 @@ > +#!/usr/bin/env bash > +# nbdkit > +# 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 > +# met: > +# > +# * Redistributions of source code must retain the above copyright > +# notice, this list of conditions and the following disclaimer. > +# > +# * Redistributions in binary form must reproduce the above copyright > +# notice, this list of conditions and the following disclaimer in the > +# documentation and/or other materials provided with the distribution. > +# > +# * Neither the name of Red Hat nor the names of its contributors may be > +# used to endorse or promote products derived from this software without > +# specific prior written permission. > +# > +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND > +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, > +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A > +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR > +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF > +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, > +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT > +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > +# SUCH DAMAGE. > + > +# Test the file plugin on readonly files. > + > +source ./functions.sh > +set -e > +set -x > + > +requires_plugin file > +requires_nbdsh_uri > +requires truncate --version > + > +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) > +files="file-readonly.pid file-readonly.img $sock" > +rm -f $files > +cleanup_fn rm -f $files > + > +truncate -s 16384 file-readonly.img > +chmod a-w file-readonly.img > +start_nbdkit -P file-readonly.pid -U $sock file file-readonly.img > + > +# Try to test all the major functions supported by both > +# the Unix and Windows versions of the file plugin. > +nbdsh -u "nbd+unix://?socket=$sock" -c ' > +assert h.is_read_only() > +assert not h.can_zero() > + > +buf = h.pread(8192, 0) > +assert buf == b"\0" * 8192 > + > +if h.can_flush(): > + h.flush() > +' > -- > 2.30.1This 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. Nir
Eric Blake
2021-Mar-02 18:25 UTC
[Libguestfs] [PATCH] file: Better support for read-only files
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
Richard W.M. Jones
2021-Mar-02 19:12 UTC
[Libguestfs] [PATCH] file: Better support for read-only files
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/