From 7512fac720f81b2ec68c34a814441dd73ff2838a Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Thu, 5 Jun 2025 10:24:33 +0100 Subject: [PATCH] Log filename, offset and count in nbdkit-file-plugin error messages resolves: RHEL-95363 --- ...lename-or-equivalent-in-the-file-han.patch | 241 ++++++++++++++++++ ...ename-or-equivalent-to-error-message.patch | 143 +++++++++++ ...e-Add-offset-count-to-error-messages.patch | 132 ++++++++++ nbdkit.spec | 9 +- 4 files changed, 524 insertions(+), 1 deletion(-) create mode 100644 0034-file-Save-the-filename-or-equivalent-in-the-file-han.patch create mode 100644 0035-file-Add-the-filename-or-equivalent-to-error-message.patch create mode 100644 0036-file-Add-offset-count-to-error-messages.patch diff --git a/0034-file-Save-the-filename-or-equivalent-in-the-file-han.patch b/0034-file-Save-the-filename-or-equivalent-in-the-file-han.patch new file mode 100644 index 0000000..211cd92 --- /dev/null +++ b/0034-file-Save-the-filename-or-equivalent-in-the-file-han.patch @@ -0,0 +1,241 @@ +From 0d7f579afc815049a6a35e1ecdd34f1857b2784b Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Wed, 4 Jun 2025 21:00:47 +0100 +Subject: [PATCH] file: Save the filename or equivalent in the file handle + +This will allow us to improve error messages. + +To make this change easier by reducing duplication, I also implemented +a common error path in the file_open function. + +(cherry picked from commit 83ad77d28d598e4b85809e7225e754e64314f802) +--- + plugins/file/file.c | 108 +++++++++++++++++++++++++------------------- + 1 file changed, 62 insertions(+), 46 deletions(-) + +diff --git a/plugins/file/file.c b/plugins/file/file.c +index b4dec3c5..f96d8a8c 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -493,6 +493,7 @@ file_list_exports (int readonly, int default_only, + /* The per-connection handle. */ + struct handle { + int fd; ++ char *name; /* Filename or equivalent. */ + struct stat statbuf; + bool is_block_device; + int sector_size; +@@ -548,7 +549,6 @@ static void * + file_open (int readonly) + { + struct handle *h; +- const char *file; + + h = malloc (sizeof *h); + if (h == NULL) { +@@ -557,36 +557,44 @@ file_open (int readonly) + } + h->can_write = !readonly; + h->fd = -1; ++ h->name = NULL; + + switch (mode) { + case mode_filename: +- file = filename; +- if (open_file_by_name (h, readonly, -1, file) == -1) { +- free (h); +- return NULL; ++ h->name = strdup (filename); ++ if (h->name == NULL) { ++ nbdkit_error ("strdup: %m"); ++ goto err; + } ++ if (open_file_by_name (h, readonly, -1, h->name) == -1) ++ goto err; + break; + + case mode_directory: { + int dfd; ++ const char *export_name = nbdkit_export_name (); + +- file = nbdkit_export_name (); +- if (strchr (file, '/')) { +- nbdkit_error ("exportname cannot contain /"); +- free (h); ++ if (export_name == NULL) ++ goto err; ++ ++ h->name = strdup (export_name); ++ if (h->name == NULL) { ++ nbdkit_error ("strdup: %m"); ++ goto err; ++ } ++ if (strchr (h->name, '/')) { ++ nbdkit_error ("%s: exportname cannot contain /", h->name); + errno = EINVAL; +- return NULL; ++ goto err; + } + dfd = open (directory, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (dfd == -1) { + nbdkit_error ("open %s: %m", directory); +- free (h); +- return NULL; ++ goto err; + } +- if (open_file_by_name (h, readonly, dfd, file) == -1) { +- free (h); ++ if (open_file_by_name (h, readonly, dfd, h->name) == -1) { + close (dfd); +- return NULL; ++ goto err; + } + close (dfd); + break; +@@ -595,14 +603,14 @@ file_open (int readonly) + case mode_fd: { + int r; + +- /* This is needed for error messages. */ +- file = ""; +- ++ if (asprintf (&h->name, "fd=%d", filedesc) == -1) { ++ nbdkit_error ("asprintf: %m"); ++ goto err; ++ } + h->fd = dup (filedesc); + if (h->fd == -1) { +- nbdkit_error ("dup fd=%d: %m", filedesc); +- free (h); +- return NULL; ++ nbdkit_error ("dup: %s: %m", h->name); ++ goto err; + } + + /* If the file descriptor is readonly then we should not advertise +@@ -610,10 +618,8 @@ file_open (int readonly) + */ + r = fcntl (h->fd, F_GETFL); + if (r == -1) { +- nbdkit_error ("fcntl: F_GETFL: %m"); +- close (h->fd); +- free (h); +- return NULL; ++ nbdkit_error ("fcntl: %s: F_GETFL: %m", h->name); ++ goto err; + } + r &= O_ACCMODE; + if (r == O_RDONLY) +@@ -627,25 +633,30 @@ file_open (int readonly) + + case mode_dirfd: { + int dfd; ++ const char *export_name = nbdkit_export_name (); + +- file = nbdkit_export_name (); +- if (strchr (file, '/')) { +- nbdkit_error ("exportname cannot contain /"); +- free (h); ++ if (export_name == NULL) ++ goto err; ++ ++ h->name = strdup (export_name); ++ if (h->name == NULL) { ++ nbdkit_error ("strdup: %m"); ++ goto err; ++ } ++ if (strchr (h->name, '/')) { ++ nbdkit_error ("%s: exportname cannot contain /", h->name); + errno = EINVAL; +- return NULL; ++ goto err; + } + /* We don't fork, so no need to worry about FD_CLOEXEC on the directory */ + dfd = dup (filedesc); + if (dfd == -1) { +- nbdkit_error ("dup dirfd=%d: %m", filedesc); +- free (h); +- return NULL; ++ nbdkit_error ("dup: dirfd=%d: %m", filedesc); ++ goto err; + } +- if (open_file_by_name (h, readonly, dfd, file) == -1) { +- free (h); ++ if (open_file_by_name (h, readonly, dfd, h->name) == -1) { + close (dfd); +- return NULL; ++ goto err; + } + close (dfd); + break; +@@ -656,12 +667,11 @@ file_open (int readonly) + } + + assert (h->fd >= 0); ++ assert (h->name != NULL); + + if (fstat (h->fd, &h->statbuf) == -1) { +- nbdkit_error ("fstat: %s: %m", file); +- close (h->fd); +- free (h); +- return NULL; ++ nbdkit_error ("fstat: %s: %m", h->name); ++ goto err; + } + + if (fadvise_mode != -1) { +@@ -669,7 +679,7 @@ file_open (int readonly) + #ifdef HAVE_POSIX_FADVISE + int r = posix_fadvise (h->fd, 0, 0, fadvise_mode); + if (r == -1) +- nbdkit_debug ("posix_fadvise: %s: %m (ignored)", file); ++ nbdkit_debug ("posix_fadvise: %s: %m (ignored)", h->name); + #else + nbdkit_debug ("fadvise is not supported"); + #endif +@@ -680,17 +690,15 @@ file_open (int readonly) + else if (S_ISREG (h->statbuf.st_mode)) + h->is_block_device = false; + else { +- nbdkit_error ("file is not regular or block device: %s", file); +- close (h->fd); +- free (h); +- return NULL; ++ nbdkit_error ("file is not regular or block device: %s", h->name); ++ goto err; + } + + h->sector_size = 4096; /* Start with safe guess */ + #ifdef BLKSSZGET + if (h->is_block_device) { + if (ioctl (h->fd, BLKSSZGET, &h->sector_size) == -1) +- nbdkit_debug ("cannot get sector size: %s: %m", file); ++ nbdkit_debug ("cannot get sector size: %s: %m", h->name); + } + #endif + +@@ -711,6 +719,13 @@ file_open (int readonly) + h->can_blkdiscard = h->is_block_device; + + return h; ++ ++ err: ++ if (h->fd >= 0) ++ close (h->fd); ++ free (h->name); ++ free (h); ++ return NULL; + } + + /* Free up the per-connection handle. */ +@@ -723,6 +738,7 @@ file_close (void *handle) + remove_fd_from_window (h->fd); + #endif + close (h->fd); ++ free (h->name); + free (h); + } + +-- +2.47.1 + diff --git a/0035-file-Add-the-filename-or-equivalent-to-error-message.patch b/0035-file-Add-the-filename-or-equivalent-to-error-message.patch new file mode 100644 index 0000000..46cf972 --- /dev/null +++ b/0035-file-Add-the-filename-or-equivalent-to-error-message.patch @@ -0,0 +1,143 @@ +From 6e5d6e5da19acfa8ba2c9fcf81651d0894f83dda Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Wed, 4 Jun 2025 21:03:58 +0100 +Subject: [PATCH] file: Add the filename (or equivalent) to error messages + +It helps to have this information when there's an error. It +particularly helps in two cases: (1) You don't have access to the +nbdkit command line. (2) The filename is derived from an export name +passed by the client. + +(cherry picked from commit 7273bd829fc0d8e492786f8908d8ddc3d5399e06) +--- + plugins/file/file.c | 28 ++++++++++++++-------------- + 1 file changed, 14 insertions(+), 14 deletions(-) + +diff --git a/plugins/file/file.c b/plugins/file/file.c +index f96d8a8c..9ba61187 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -754,7 +754,7 @@ file_get_size (void *handle) + + r = device_size (h->fd, &h->statbuf); + if (r == -1) { +- nbdkit_error ("device_size: %m"); ++ nbdkit_error ("device_size: %s: %m", h->name); + return -1; + } + return r; +@@ -818,7 +818,7 @@ file_flush (void *handle, uint32_t flags) + struct handle *h = handle; + + if (fdatasync (h->fd) == -1) { +- nbdkit_error ("fdatasync: %m"); ++ nbdkit_error ("fdatasync: %s: %m", h->name); + return -1; + } + +@@ -839,11 +839,11 @@ file_pread (void *handle, void *buf, uint32_t count, uint64_t offset, + while (count > 0) { + ssize_t r = pread (h->fd, buf, count, offset); + if (r == -1) { +- nbdkit_error ("pread: %m"); ++ nbdkit_error ("pread: %s: %m", h->name); + return -1; + } + if (r == 0) { +- nbdkit_error ("pread: unexpected end of file"); ++ nbdkit_error ("pread: %s: unexpected end of file", h->name); + return -1; + } + buf += r; +@@ -875,7 +875,7 @@ file_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, + while (count > 0) { + ssize_t r = pwrite (h->fd, buf, count, offset); + if (r == -1) { +- nbdkit_error ("pwrite: %m"); ++ nbdkit_error ("pwrite: %s: %m", h->name); + return -1; + } + buf += r; +@@ -957,7 +957,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -993,7 +993,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -1001,7 +1001,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + else { + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -1023,7 +1023,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -1051,14 +1051,14 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + + h->can_fallocate = false; + } else { + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -1082,7 +1082,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (errno != ENOTTY) { +- nbdkit_error ("zero: %m"); ++ nbdkit_error ("zero: %s: %m", h->name); + return -1; + } + +@@ -1120,7 +1120,7 @@ file_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + /* Trim is advisory; we don't care if it fails for anything other + * than EIO or EPERM. */ + if (errno == EPERM || errno == EIO) { +- nbdkit_error ("fallocate: %m"); ++ nbdkit_error ("fallocate: %s: %m", h->name); + return -1; + } + +@@ -1241,7 +1241,7 @@ file_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + r = posix_fadvise (h->fd, offset, count, POSIX_FADV_WILLNEED); + if (r) { + errno = r; +- nbdkit_error ("posix_fadvise: %m"); ++ nbdkit_error ("posix_fadvise: %s: %m", h->name); + return -1; + } + return 0; +-- +2.47.1 + diff --git a/0036-file-Add-offset-count-to-error-messages.patch b/0036-file-Add-offset-count-to-error-messages.patch new file mode 100644 index 0000000..665780e --- /dev/null +++ b/0036-file-Add-offset-count-to-error-messages.patch @@ -0,0 +1,132 @@ +From 45f740e3b4a707e5d2db739b16fb33f4d7b466cf Mon Sep 17 00:00:00 2001 +From: "Richard W.M. Jones" +Date: Wed, 4 Jun 2025 21:15:26 +0100 +Subject: [PATCH] file: Add offset/count to error messages + +Although the same information is available in debug logs, if verbose +messages (-v) are not used then the information is lost. As it might +be useful occasionally, ensure it is captured in errors. + +(cherry picked from commit 253574354252f4a77e2df0769b721e76f1777651) +--- + plugins/file/file.c | 35 ++++++++++++++++++++++++----------- + 1 file changed, 24 insertions(+), 11 deletions(-) + +diff --git a/plugins/file/file.c b/plugins/file/file.c +index 9ba61187..ade6f5ff 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -839,7 +839,8 @@ file_pread (void *handle, void *buf, uint32_t count, uint64_t offset, + while (count > 0) { + ssize_t r = pread (h->fd, buf, count, offset); + if (r == -1) { +- nbdkit_error ("pread: %s: %m", h->name); ++ nbdkit_error ("pread: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + if (r == 0) { +@@ -875,7 +876,8 @@ file_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, + while (count > 0) { + ssize_t r = pwrite (h->fd, buf, count, offset); + if (r == -1) { +- nbdkit_error ("pwrite: %s: %m", h->name); ++ nbdkit_error ("pwrite: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + buf += r; +@@ -957,7 +959,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + +@@ -993,7 +996,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + +@@ -1001,7 +1005,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + else { + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + +@@ -1023,7 +1028,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + +@@ -1051,14 +1057,16 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + + h->can_fallocate = false; + } else { + if (!is_enotsup (errno)) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ":%m", ++ h->name, offset, count); + return -1; + } + +@@ -1082,7 +1090,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + } + + if (errno != ENOTTY) { +- nbdkit_error ("zero: %s: %m", h->name); ++ nbdkit_error ("zero: %s: offset=%" PRIu64 ", count=%" PRIu32 ": %m", ++ h->name, offset, count); + return -1; + } + +@@ -1120,7 +1129,9 @@ file_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + /* Trim is advisory; we don't care if it fails for anything other + * than EIO or EPERM. */ + if (errno == EPERM || errno == EIO) { +- nbdkit_error ("fallocate: %s: %m", h->name); ++ nbdkit_error ("fallocate: %s: offset=%" PRIu64 ", count=%" PRIu32 ":" ++ " %m", ++ h->name, offset, count); + return -1; + } + +@@ -1241,7 +1252,9 @@ file_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) + r = posix_fadvise (h->fd, offset, count, POSIX_FADV_WILLNEED); + if (r) { + errno = r; +- nbdkit_error ("posix_fadvise: %s: %m", h->name); ++ nbdkit_error ("posix_fadvise: %s: offset=%" PRIu64 ", count=%" PRIu32 ":" ++ " %m", ++ h->name, offset, count); + return -1; + } + return 0; +-- +2.47.1 + diff --git a/nbdkit.spec b/nbdkit.spec index ad80cca..127a761 100644 --- a/nbdkit.spec +++ b/nbdkit.spec @@ -56,7 +56,7 @@ Name: nbdkit Version: 1.38.5 -Release: 7%{?dist} +Release: 8%{?dist} Summary: NBD server License: BSD-3-Clause @@ -114,6 +114,9 @@ Patch0030: 0030-vddk-Cache-the-readonly-flag-from-the-.open-call-in-.patch Patch0031: 0031-vddk-Move-minimum-version-of-VDDK-to-6.7.patch Patch0032: 0032-vddk-Unconditionally-test-QueryAllocatedBlocks.patch Patch0033: 0033-vddk-Pre-cache-the-extents-for-readonly-connections.patch +Patch0034: 0034-file-Save-the-filename-or-equivalent-in-the-file-han.patch +Patch0035: 0035-file-Add-the-filename-or-equivalent-to-error-message.patch +Patch0036: 0036-file-Add-offset-count-to-error-messages.patch # For automatic RPM Provides generation. # See: https://rpm-software-management.github.io/rpm/manual/dependency_generators.html @@ -1532,6 +1535,10 @@ fi %changelog +* Thu Jun 05 2025 Richard W.M. Jones - 1.38.5-8 +- Log filename, offset and count in nbdkit-file-plugin error messages + resolves: RHEL-95363 + * Mon Jun 02 2025 Richard W.M. Jones - 1.38.5-7 - vddk: Pre-cache the extents for readonly connections resolves: RHEL-94823