From 049a1e51bf0d407db86fc3c8351ac00977963086 Mon Sep 17 00:00:00 2001 From: John Kerl Date: Thu, 23 Jan 2025 11:59:13 -0500 Subject: [PATCH 1/5] Show reasons a POSIX file cannot be read --- tiledb/sm/filesystem/mem_filesystem.cc | 9 +++++++-- tiledb/sm/filesystem/posix.cc | 14 +++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/tiledb/sm/filesystem/mem_filesystem.cc b/tiledb/sm/filesystem/mem_filesystem.cc index b1de887c305..a398b801bf5 100644 --- a/tiledb/sm/filesystem/mem_filesystem.cc +++ b/tiledb/sm/filesystem/mem_filesystem.cc @@ -31,6 +31,7 @@ */ #include +#include #include #include #include @@ -183,8 +184,12 @@ class MemFilesystem::File : public MemFilesystem::FSNode { assert(buffer); if (offset + nbytes > size_) - return LOG_STATUS( - Status_MemFSError("Cannot read from file; Read exceeds file size")); + return LOG_STATUS(Status_MemFSError(std::format( + "Cannot read from file; Read exceeds file size: offset {} nbytes {} " + "size_ {}", + offset, + nbytes, + size_))); memcpy(buffer, (char*)data_ + offset, nbytes); return Status::Ok(); diff --git a/tiledb/sm/filesystem/posix.cc b/tiledb/sm/filesystem/posix.cc index 8b9cd679605..1fcb673056d 100644 --- a/tiledb/sm/filesystem/posix.cc +++ b/tiledb/sm/filesystem/posix.cc @@ -32,10 +32,10 @@ #ifndef _WIN32 -#include "tiledb/sm/filesystem/posix.h" #include "tiledb/common/filesystem/directory_entry.h" #include "tiledb/common/logger.h" #include "tiledb/common/stdx_string.h" +#include "tiledb/sm/filesystem/posix.h" #include "tiledb/sm/misc/constants.h" #include "tiledb/sm/misc/tdb_math.h" #include "uri.h" @@ -47,6 +47,7 @@ #include #include +#include #include #include #include @@ -273,8 +274,15 @@ void Posix::read( auto path = uri.to_path(); uint64_t file_size; this->file_size(URI(path), &file_size); - if (offset + nbytes > file_size) - throw IOError("Cannot read from file; Read exceeds file size"); + if (offset + nbytes > file_size) { + throw IOError(std::format( + "Cannot read from file; Read exceeds file size: offset {}, nbytes {}, " + "file_size {}, URI {}", + offset, + nbytes, + file_size, + uri.to_path())); + } // Open file int fd = open(path.c_str(), O_RDONLY); From 7d797f160719b2b987ea45a2d25cb627487cb09c Mon Sep 17 00:00:00 2001 From: John Kerl Date: Thu, 23 Jan 2025 12:06:38 -0500 Subject: [PATCH 2/5] Switch to clang-format==17.0.6 for core --- tiledb/sm/filesystem/posix.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiledb/sm/filesystem/posix.cc b/tiledb/sm/filesystem/posix.cc index 1fcb673056d..f3fb28fdd6a 100644 --- a/tiledb/sm/filesystem/posix.cc +++ b/tiledb/sm/filesystem/posix.cc @@ -32,10 +32,10 @@ #ifndef _WIN32 +#include "tiledb/sm/filesystem/posix.h" #include "tiledb/common/filesystem/directory_entry.h" #include "tiledb/common/logger.h" #include "tiledb/common/stdx_string.h" -#include "tiledb/sm/filesystem/posix.h" #include "tiledb/sm/misc/constants.h" #include "tiledb/sm/misc/tdb_math.h" #include "uri.h" From c04280b493354c0734e99437907ab98b53c6ac70 Mon Sep 17 00:00:00 2001 From: John Kerl Date: Thu, 23 Jan 2025 14:01:40 -0500 Subject: [PATCH 3/5] use `fmt::format` --- tiledb/sm/filesystem/mem_filesystem.cc | 3 +-- tiledb/sm/filesystem/posix.cc | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tiledb/sm/filesystem/mem_filesystem.cc b/tiledb/sm/filesystem/mem_filesystem.cc index a398b801bf5..439b535d69e 100644 --- a/tiledb/sm/filesystem/mem_filesystem.cc +++ b/tiledb/sm/filesystem/mem_filesystem.cc @@ -31,7 +31,6 @@ */ #include -#include #include #include #include @@ -184,7 +183,7 @@ class MemFilesystem::File : public MemFilesystem::FSNode { assert(buffer); if (offset + nbytes > size_) - return LOG_STATUS(Status_MemFSError(std::format( + return LOG_STATUS(Status_MemFSError(fmt::format( "Cannot read from file; Read exceeds file size: offset {} nbytes {} " "size_ {}", offset, diff --git a/tiledb/sm/filesystem/posix.cc b/tiledb/sm/filesystem/posix.cc index f3fb28fdd6a..30e62eeeee9 100644 --- a/tiledb/sm/filesystem/posix.cc +++ b/tiledb/sm/filesystem/posix.cc @@ -47,7 +47,6 @@ #include #include -#include #include #include #include @@ -275,7 +274,7 @@ void Posix::read( uint64_t file_size; this->file_size(URI(path), &file_size); if (offset + nbytes > file_size) { - throw IOError(std::format( + throw IOError(fmt::format( "Cannot read from file; Read exceeds file size: offset {}, nbytes {}, " "file_size {}, URI {}", offset, From 097babfbecf0ebfbef9cc62192e598497aa4d719 Mon Sep 17 00:00:00 2001 From: John Kerl Date: Fri, 24 Jan 2025 12:28:48 -0500 Subject: [PATCH 4/5] same for windows --- tiledb/sm/filesystem/win.cc | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/tiledb/sm/filesystem/win.cc b/tiledb/sm/filesystem/win.cc index 684a5c6ea51..95b12975b6f 100644 --- a/tiledb/sm/filesystem/win.cc +++ b/tiledb/sm/filesystem/win.cc @@ -462,11 +462,19 @@ Status Win::read( 0) { auto gle = GetLastError(); CloseHandle(file_h); + + std::string err_msg; + if (gle != 0) { + err_msg = get_last_error_msg(gle, "ReadFile"); + } else { + err_msg = "num_bytes_read " + std::to_string(num_bytes_read) + + " != nbytes " + std::to_string(nbytes) + } + return LOG_STATUS(Status_IOError( - "Cannot read from file '" + path + "'; File read error " + - (gle != 0 ? get_last_error_msg(gle, "ReadFile") : - "num_bytes_read " + std::to_string(num_bytes_read) + - " != nbyes " + std::to_string(nbytes)))); + "Cannot read from file '" + path + "'; File read error '" + err_msg + + "' offset " + std::string(offset) + " nbytes " + + std::string(nbytes))); } byte_buffer += num_bytes_read; offset += num_bytes_read; @@ -548,8 +556,9 @@ Status Win::write( uint64_t file_offset = file_size_lg_int.QuadPart; if (!write_at(file_h, file_offset, buffer, buffer_size).ok()) { CloseHandle(file_h); - return LOG_STATUS( - Status_IOError(std::string("Cannot write to file '") + path)); + return LOG_STATUS(Status_IOError( + std::string("Cannot write to file '") + path + "'" + " file_offset " + + std::string(file_offset) + " buffer_size " + std::string(buffer_size))); } // Always close the handle. if (CloseHandle(file_h) == 0) { @@ -588,9 +597,11 @@ Status Win::write_at( bytes_to_write, &bytes_written, &ov) == 0) { + // There's no guarantee that the bytes_written outarg was updated, when + // the write failed -- so let's not log it. return LOG_STATUS(Status_IOError(std::string( - "Cannot write to file; File writing error: " + - get_last_error_msg("WriteFile")))); + get_last_error_msg("WriteFile") + " bytes_to_write " + + std::string(bytes_to_write)))); } remaining_bytes_to_write -= bytes_written; byte_idx += bytes_written; From 02fcff8fc90f1f4b2661125eac66857c5ddbb6fe Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 24 Jan 2025 22:15:44 +0200 Subject: [PATCH 5/5] FIx compile errors on Windows, and revert the error message changes on write. --- tiledb/sm/filesystem/win.cc | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/tiledb/sm/filesystem/win.cc b/tiledb/sm/filesystem/win.cc index 95b12975b6f..6b851a1f6ff 100644 --- a/tiledb/sm/filesystem/win.cc +++ b/tiledb/sm/filesystem/win.cc @@ -58,6 +58,8 @@ #include "uri.h" #include "win.h" +#include + using namespace tiledb::common; using tiledb::common::filesystem::directory_entry; @@ -467,14 +469,18 @@ Status Win::read( if (gle != 0) { err_msg = get_last_error_msg(gle, "ReadFile"); } else { - err_msg = "num_bytes_read " + std::to_string(num_bytes_read) + - " != nbytes " + std::to_string(nbytes) + err_msg = std::string("num_bytes_read ") + + std::to_string(num_bytes_read) + " != nbytes " + + std::to_string(nbytes); } - return LOG_STATUS(Status_IOError( - "Cannot read from file '" + path + "'; File read error '" + err_msg + - "' offset " + std::string(offset) + " nbytes " + - std::string(nbytes))); + return LOG_STATUS(Status_IOError(fmt::format( + "Cannot read from file '{}'; File read error '{}' offset {} nbytes " + "{}", + path, + err_msg, + offset, + nbytes))); } byte_buffer += num_bytes_read; offset += num_bytes_read; @@ -556,9 +562,8 @@ Status Win::write( uint64_t file_offset = file_size_lg_int.QuadPart; if (!write_at(file_h, file_offset, buffer, buffer_size).ok()) { CloseHandle(file_h); - return LOG_STATUS(Status_IOError( - std::string("Cannot write to file '") + path + "'" + " file_offset " + - std::string(file_offset) + " buffer_size " + std::string(buffer_size))); + return LOG_STATUS( + Status_IOError(std::string("Cannot write to file '") + path)); } // Always close the handle. if (CloseHandle(file_h) == 0) { @@ -597,11 +602,9 @@ Status Win::write_at( bytes_to_write, &bytes_written, &ov) == 0) { - // There's no guarantee that the bytes_written outarg was updated, when - // the write failed -- so let's not log it. return LOG_STATUS(Status_IOError(std::string( - get_last_error_msg("WriteFile") + " bytes_to_write " + - std::string(bytes_to_write)))); + "Cannot write to file; File writing error: " + + get_last_error_msg("WriteFile")))); } remaining_bytes_to_write -= bytes_written; byte_idx += bytes_written;