From 06e47220747ebdaa51b68a09a50f3f907e668647 Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Thu, 5 Jun 2025 10:24:35 +0100 Subject: [PATCH] Log filename, offset and count in nbdkit-file-plugin error messages resolves: RHEL-95364 --- ...lename-or-equivalent-in-the-file-han.patch | 263 ++++++++++++++++++ ...ename-or-equivalent-to-error-message.patch | 143 ++++++++++ ...e-Add-offset-count-to-error-messages.patch | 132 +++++++++ nbdkit.spec | 9 +- 4 files changed, 545 insertions(+), 2 deletions(-) create mode 100644 0024-file-Save-the-filename-or-equivalent-in-the-file-han.patch create mode 100644 0025-file-Add-the-filename-or-equivalent-to-error-message.patch create mode 100644 0026-file-Add-offset-count-to-error-messages.patch diff --git a/0024-file-Save-the-filename-or-equivalent-in-the-file-han.patch b/0024-file-Save-the-filename-or-equivalent-in-the-file-han.patch new file mode 100644 index 0000000..d8c64b9 --- /dev/null +++ b/0024-file-Save-the-filename-or-equivalent-in-the-file-han.patch @@ -0,0 +1,263 @@ +From 0f2fc18c423dc3f5aef5a2d38c918905b4fbc6ef 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 | 114 +++++++++++++++++++++++++------------------- + 1 file changed, 65 insertions(+), 49 deletions(-) + +diff --git a/plugins/file/file.c b/plugins/file/file.c +index dc100bb3..4dec4bd9 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -500,6 +500,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; +@@ -557,7 +558,6 @@ static void * + file_open (int readonly) + { + struct handle *h; +- const char *file; + + h = malloc (sizeof *h); + if (h == NULL) { +@@ -566,36 +566,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; +@@ -604,14 +612,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 +@@ -619,10 +627,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) +@@ -636,25 +642,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; +@@ -665,12 +676,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) { +@@ -678,7 +688,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 +@@ -689,17 +699,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 + +@@ -707,7 +715,7 @@ file_open (int readonly) + #ifdef BLKROTATIONAL + if (h->is_block_device) { + if (ioctl (h->fd, BLKROTATIONAL, &h->rotational) == -1) +- nbdkit_debug ("cannot get rotational property: %s: %m", file); ++ nbdkit_debug ("cannot get rotational property: %s: %m", h->name); + } + #endif + +@@ -717,10 +725,10 @@ file_open (int readonly) + unsigned int minimum_io_size = 0, optimal_io_size = 0; + + if (ioctl (h->fd, BLKIOMIN, &minimum_io_size) == -1) +- nbdkit_debug ("cannot get BLKIOMIN: %s: %m", file); ++ nbdkit_debug ("cannot get BLKIOMIN: %s: %m", h->name); + + if (ioctl (h->fd, BLKIOOPT, &optimal_io_size) == -1) +- nbdkit_debug ("cannot get BLKIOOPT: %s: %m", file); ++ nbdkit_debug ("cannot get BLKIOOPT: %s: %m", h->name); + else if (optimal_io_size == 0) + /* All devices in the Linux kernel except for MD report optimal + * as 0. In that case guess a good value. +@@ -754,6 +762,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. */ +@@ -766,6 +781,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/0025-file-Add-the-filename-or-equivalent-to-error-message.patch b/0025-file-Add-the-filename-or-equivalent-to-error-message.patch new file mode 100644 index 0000000..fd7f6b5 --- /dev/null +++ b/0025-file-Add-the-filename-or-equivalent-to-error-message.patch @@ -0,0 +1,143 @@ +From 53f5fa4e1f560475dadbefc1f00e0f3d2ca84ca0 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 4dec4bd9..8018b294 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -797,7 +797,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; +@@ -883,7 +883,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; + } + +@@ -904,11 +904,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; +@@ -940,7 +940,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; +@@ -1022,7 +1022,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; + } + +@@ -1058,7 +1058,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; + } + +@@ -1066,7 +1066,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; + } + +@@ -1088,7 +1088,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; + } + +@@ -1116,14 +1116,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; + } + +@@ -1147,7 +1147,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; + } + +@@ -1185,7 +1185,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; + } + +@@ -1306,7 +1306,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/0026-file-Add-offset-count-to-error-messages.patch b/0026-file-Add-offset-count-to-error-messages.patch new file mode 100644 index 0000000..e687e2d --- /dev/null +++ b/0026-file-Add-offset-count-to-error-messages.patch @@ -0,0 +1,132 @@ +From 817bacccc1f44a060a4250ff9e6a27086be09d8b 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 8018b294..e62e2437 100644 +--- a/plugins/file/file.c ++++ b/plugins/file/file.c +@@ -904,7 +904,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) { +@@ -940,7 +941,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; +@@ -1022,7 +1024,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; + } + +@@ -1058,7 +1061,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; + } + +@@ -1066,7 +1070,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; + } + +@@ -1088,7 +1093,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; + } + +@@ -1116,14 +1122,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; + } + +@@ -1147,7 +1155,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; + } + +@@ -1185,7 +1194,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; + } + +@@ -1306,7 +1317,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 c81c1e4..1e694b6 100644 --- a/nbdkit.spec +++ b/nbdkit.spec @@ -55,7 +55,7 @@ Name: nbdkit Version: 1.42.2 -Release: 5%{?dist} +Release: 6%{?dist} Summary: NBD server License: BSD-3-Clause @@ -103,6 +103,9 @@ Patch0020: 0020-vddk-Cache-the-readonly-flag-from-the-.open-call-in-.patch Patch0021: 0021-vddk-Move-minimum-version-of-VDDK-to-6.7.patch Patch0022: 0022-vddk-Unconditionally-test-QueryAllocatedBlocks.patch Patch0023: 0023-vddk-Pre-cache-the-extents-for-readonly-connections.patch +Patch0024: 0024-file-Save-the-filename-or-equivalent-in-the-file-han.patch +Patch0025: 0025-file-Add-the-filename-or-equivalent-to-error-message.patch +Patch0026: 0026-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 @@ -1545,7 +1548,7 @@ fi %changelog -* Mon Jun 02 2025 Richard W.M. Jones - 1.42.2-5 +* Thu Jun 05 2025 Richard W.M. Jones - 1.42.2-6 - Rebase to nbdkit 1.42.2 - Synch the spec file with Fedora Rawhide. - nbdkit-ondemand-plugin moves into a new subpackage. @@ -1557,6 +1560,8 @@ fi resolves: RHEL-89371 - vddk: Pre-cache the extents for readonly connections resolves: RHEL-94825 +- Log filename, offset and count in nbdkit-file-plugin error messages + resolves: RHEL-95364 * Mon Jan 06 2025 Richard W.M. Jones - 1.40.4-3 - vddk: Avoid reading partial chunk beyond the end of the disk