Eric Blake
2021-Mar-02 14:44 UTC
[Libguestfs] [PATCH] file: Better support for read-only files
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.
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);
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.1
Richard W.M. Jones
2021-Mar-02 15:01 UTC
[Libguestfs] [PATCH] file: Better support for read-only files
On Tue, Mar 02, 2021 at 08:44:04AM -0600, Eric Blake 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. > > The windows code compiles, but I wasn't able to test it as thoroughly > as the Unix code.> +static int > +wintfile_can_write (void *handle) > +{ > + struct handle *h = handle; > + return !h->is_readonly; > +}Actually it doesn't because there's a misspelling. I modified the patch s/wintfile/winfile/, but the newly added test still fails on Wine with: nbdkit: file[1]: debug: file: flush nbdkit: file[1]: error: Z:\home\rjones\d\nbdkit-windows\tests\file-readonly.img: FlushFileBuffers: 5 nbdkit: file[1]: debug: sending error reply: Input/output error ... libnbd: debug: nbdsh: nbd_flush: leave: error="nbd_flush: flush: command failed: Input/output error" ... Traceback (most recent call last): File "/usr/lib64/python3.9/site-packages/nbdsh.py", line 119, in shell exec(c, d, d) File "<string>", line 9, in <module> File "/usr/lib64/python3.9/site-packages/nbd.py", line 1608, in flush return libnbdmod.flush(self._o, flags) nbd.Error: nbd_flush: flush: command failed: Input/output error (EIO) My guess is that Win32 doesn't let you flush a read-only file! The actual error is ERROR_ACCESS_DENIED (== 5). The attached patch fixes both issues. Rest of the patch is fine, so ACK. 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/ -------------- next part -------------->From 69bba02f382eb757db4a74136beb9cf45d715842 Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones at redhat.com> Date: Tue, 2 Mar 2021 14:59:35 +0000 Subject: [PATCH] UPDATE: "file: Better support for read-only files" --- plugins/file/winfile.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/plugins/file/winfile.c b/plugins/file/winfile.c index ec186255..04018bfa 100644 --- a/plugins/file/winfile.c +++ b/plugins/file/winfile.c @@ -198,7 +198,18 @@ winfile_open (int readonly) } static int -wintfile_can_write (void *handle) +winfile_can_write (void *handle) +{ + struct handle *h = handle; + return !h->is_readonly; +} + +/* Windows cannot flush on a read-only file. It returns + * ERROR_ACCESS_DENIED. Therefore don't advertise flush if the handle + * is r/o. + */ +static int +winfile_can_flush (void *handle) { struct handle *h = handle; return !h->is_readonly; @@ -442,6 +453,7 @@ static struct nbdkit_plugin plugin = { .open = winfile_open, .can_write = winfile_can_write, + .can_flush = winfile_can_flush, .can_trim = winfile_can_trim, .can_zero = winfile_can_zero, .can_extents = winfile_can_extents, -- 2.29.0.rc2
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