From 8c0bb23379d1f3d392f6eb4efcb16b5b4e20a7e3 Mon Sep 17 00:00:00 2001 From: Chris Hogan Date: Wed, 7 Apr 2021 16:11:41 -0500 Subject: [PATCH] Another error handling pass --- src/api/bucket.cc | 3 - src/api/vbucket.cc | 11 +-- src/buffer_pool.cc | 160 +++++++++++++++++-------------------- src/config_parser.cc | 11 +-- src/memory_management.cc | 20 ++--- src/metadata_management.cc | 2 +- src/utils.cc | 5 ++ src/utils.h | 2 + 8 files changed, 98 insertions(+), 116 deletions(-) diff --git a/src/api/bucket.cc b/src/api/bucket.cc index 52f0c65a3..1c7b9d746 100644 --- a/src/api/bucket.cc +++ b/src/api/bucket.cc @@ -145,7 +145,6 @@ Status Bucket::RenameBlob(const std::string &old_name, Status ret; if (IsBlobNameTooLong(new_name)) { - // TODO(chogan): @errorhandling ret = BLOB_NAME_TOO_LONG; LOG(ERROR) << ret.Msg(); return ret; @@ -197,7 +196,6 @@ Status Bucket::Rename(const std::string &new_name, Context &ctx) { Status ret; if (IsBucketNameTooLong(new_name)) { - // TODO(chogan): @errorhandling ret = BUCKET_NAME_TOO_LONG; LOG(ERROR) << ret.Msg(); return ret; @@ -248,7 +246,6 @@ Status Bucket::Destroy(Context &ctx) { if (destroyed) { id_.as_int = 0; } else { - // TODO(chogan): @errorhandling result = BUCKET_IN_USE; LOG(ERROR) << result.Msg(); } diff --git a/src/api/vbucket.cc b/src/api/vbucket.cc index 8affcdd75..f0e6dede5 100644 --- a/src/api/vbucket.cc +++ b/src/api/vbucket.cc @@ -49,7 +49,6 @@ Status VBucket::Link(std::string blob_name, std::string bucket_name, } } } else { - // TODO(hari): @errorhandling ret = BLOB_NOT_IN_BUCKET; LOG(ERROR) << ret.Msg(); } @@ -82,7 +81,6 @@ Status VBucket::Unlink(std::string blob_name, std::string bucket_name, } } if (!found) { - // TODO(hari): @errorhandling ret = BLOB_NOT_LINKED_TO_VBUCKET; LOG(ERROR) << ret.Msg(); } @@ -152,7 +150,6 @@ Status VBucket::Attach(Trait* trait, Context& ctx) { } attached_traits_.push_back(trait); } else { - // TODO(hari): @errorhandling throw trait already exists. ret = TRAIT_EXISTS_ALREADY; LOG(ERROR) << ret.Msg(); } @@ -192,7 +189,6 @@ Status VBucket::Detach(Trait* trait, Context& ctx) { } attached_traits_.erase(selected_trait_iter); } else { - // TODO(hari): @errorhandling throw trait not valid. ret = TRAIT_NOT_VALID; LOG(ERROR) << ret.Msg(); } @@ -260,20 +256,18 @@ Status VBucket::Delete(Context& ctx) { auto blob_id = GetBlobId(&hermes_->context_, &hermes_->rpc_, ci->second, bucket_id); - // TODO(hari): @errorhandling check return of StdIoPersistBlob ret = StdIoPersistBlob(&hermes_->context_, &hermes_->rpc_, &hermes_->trans_arena_, blob_id, file, iter->second); - if (!ret.Succeeded()) + if (!ret.Succeeded()) { LOG(ERROR) << ret.Msg(); + } } else { - // TODO(hari): @errorhandling map doesnt have the blob linked. ret = BLOB_NOT_LINKED_IN_MAP; LOG(ERROR) << ret.Msg(); } } else { - // TODO(hari): @errorhandling offset_map should not be empty ret = OFFSET_MAP_EMPTY; LOG(ERROR) << ret.Msg(); } @@ -294,7 +288,6 @@ Status VBucket::Delete(Context& ctx) { if (file != nullptr) { fflush(file); if (fclose(file) != 0) { - // TODO(chogan): @errorhandling ret = FCLOSE_FAILED; LOG(ERROR) << ret.Msg() << strerror(errno); } diff --git a/src/buffer_pool.cc b/src/buffer_pool.cc index 52ce933bd..5d71a9b7c 100644 --- a/src/buffer_pool.cc +++ b/src/buffer_pool.cc @@ -732,10 +732,13 @@ Device *InitDevices(Arena *arena, Config *config) { if (path_length == 0) { device->is_byte_addressable = true; } else { - // TODO(chogan): @errorhandling - assert(path_length < kMaxPathLength); - snprintf(device->mount_point, path_length + 1, "%s", - config->mount_points[i].c_str()); + if (path_length < kMaxPathLength) { + snprintf(device->mount_point, path_length + 1, "%s", + config->mount_points[i].c_str()); + } else { + LOG(ERROR) << "Mount point path " << config->mount_points[i] + << " exceeds max length of " << kMaxPathLength << std::endl; + } } } @@ -1183,8 +1186,7 @@ void SerializeBufferPoolToFile(SharedMemoryContext *context, FILE *file) { int result = fwrite(context->shm_base, context->shm_size, 1, file); if (result < 1) { - // TODO(chogan): @errorhandling - perror("Failed to serialize BufferPool to file"); + FailedLibraryCall("fwrite"); } } @@ -1196,11 +1198,17 @@ void MakeFullShmemName(char *dest, const char *base) { char *username = getenv("USER"); if (username) { size_t username_length = strlen(username); - [[maybe_unused]] size_t total_length = - base_name_length + username_length + 1; - // TODO(chogan): @errorhandling - assert(total_length < kMaxBufferPoolShmemNameLength); - snprintf(dest + base_name_length, username_length + 1, "%s", username); + size_t total_length = base_name_length + username_length + 1; + + if (total_length < kMaxBufferPoolShmemNameLength) { + snprintf(dest + base_name_length, username_length + 1, "%s", username); + } else { + LOG(ERROR) << "Shared memory name " << base << username + << " exceeds max length of " << kMaxBufferPoolShmemNameLength + << std::endl; + } + } else { + // TODO(chogan): Use pid? } } @@ -1283,22 +1291,24 @@ u8 *InitSharedMemory(const char *shmem_name, size_t total_size) { shm_open(shmem_name, O_CREAT | O_RDWR | O_TRUNC, S_IRUSR | S_IWUSR); if (shmem_fd >= 0) { - [[maybe_unused]] int ftruncate_result = ftruncate(shmem_fd, total_size); - // TODO(chogan): @errorhandling - assert(ftruncate_result == 0); + int ftruncate_result = ftruncate(shmem_fd, total_size); + if (ftruncate_result != 0) { + FailedLibraryCall("ftruncate"); + } result = (u8 *)mmap(0, total_size, PROT_READ | PROT_WRITE, MAP_SHARED, shmem_fd, 0); - // TODO(chogan): @errorhandling - close(shmem_fd); + + if (result == MAP_FAILED) { + FailedLibraryCall("mmap"); + } + if (close(shmem_fd) == -1) { + FailedLibraryCall("close"); + } } else { - // TODO(chogan): @errorhandling - assert(!"shm_open failed\n"); + FailedLibraryCall("shm_open"); } - // TODO(chogan): @errorhandling - assert(result); - return result; } @@ -1353,8 +1363,7 @@ void CloseBufferingFiles(SharedMemoryContext *context) { if (context->open_streams[device_id][slab]) { int fclose_result = fclose(context->open_streams[device_id][slab]); if (fclose_result != 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; + FailedLibraryCall("fclose"); } } } @@ -1362,8 +1371,7 @@ void CloseBufferingFiles(SharedMemoryContext *context) { if (context->swap_file) { if (fclose(context->swap_file) != 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; + FailedLibraryCall("fclose"); } } } @@ -1405,12 +1413,12 @@ size_t LocalWriteBufferById(SharedMemoryContext *context, BufferID id, context->open_streams[device->id][slab_index] = file; } fseek(file, header->data_offset, SEEK_SET); - [[maybe_unused]] size_t items_written = fwrite(at, write_size, 1, file); - // TODO(chogan): @errorhandling - assert(items_written == 1); + size_t items_written = fwrite(at, write_size, 1, file); + if (items_written != 1) { + FailedLibraryCall("fwrite"); + } if (fflush(file) != 0) { - // TODO(chogan): @errorhandling - LOG(WARNING) << "fflush failed\n"; + FailedLibraryCall("fflush"); } // fsync(fileno(file)); } @@ -1480,8 +1488,9 @@ size_t LocalReadBufferById(SharedMemoryContext *context, BufferID id, fseek(file, header->data_offset, SEEK_SET); size_t items_read = fread((u8 *)blob->data + read_offset, read_size, 1, file); - // TODO(chogan): @errorhandling - assert(items_read == 1); + if (items_read != 1) { + FailedLibraryCall("fwrite"); + } result = items_read * read_size; } UnlockBuffer(header); @@ -1505,8 +1514,10 @@ size_t ReadBlobFromBuffers(SharedMemoryContext *context, RpcContext *rpc, "RemoteBulkReadBufferById", blob->data + total_bytes_read, buffer_sizes[i], id); - // TODO(chogan): @errorhandling - assert(bytes_transferred == buffer_sizes[i]); + if (bytes_transferred != buffer_sizes[i]) { + LOG(ERROR) << "BulkRead only transferred " << bytes_transferred + << " out of " << buffer_sizes[i] << " bytes" << std::endl; + } bytes_read += bytes_transferred; } else { std::vector data = @@ -1522,8 +1533,11 @@ size_t ReadBlobFromBuffers(SharedMemoryContext *context, RpcContext *rpc, } total_bytes_read += bytes_read; } - // TODO(chogan): @errorhandling - assert(total_bytes_read == blob->size); + + if (total_bytes_read != blob->size) { + LOG(ERROR) << __func__ << "expected to read a Blob of size " << blob->size + << " but only read " << total_bytes_read << std::endl; + } return total_bytes_read; } @@ -1551,52 +1565,39 @@ size_t ReadBlobById(SharedMemoryContext *context, RpcContext *rpc, Arena *arena, return result; } -int OpenSwapFile(SharedMemoryContext *context, u32 node_id) { - int result = 0; - +void OpenSwapFile(SharedMemoryContext *context, u32 node_id) { if (!context->swap_file) { MetadataManager *mdm = GetMetadataManagerFromContext(context); std::string swap_path = GetSwapFilename(mdm, node_id); context->swap_file = fopen(swap_path.c_str(), "a+"); if (!context->swap_file) { - // TODO(chogan): @errorhandling - result = 1; + FailedLibraryCall("fopen"); } } - - return result; } SwapBlob WriteToSwap(SharedMemoryContext *context, Blob blob, u32 node_id, BucketID bucket_id) { SwapBlob result = {}; - if (OpenSwapFile(context, node_id) == 0) { - if (fseek(context->swap_file, 0, SEEK_END) != 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } + OpenSwapFile(context, node_id); + if (fseek(context->swap_file, 0, SEEK_END) != 0) { + FailedLibraryCall("fseek"); + } - long int file_position = ftell(context->swap_file); - if (file_position == -1) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } - result.offset = file_position; + long int file_position = ftell(context->swap_file); + if (file_position == -1) { + FailedLibraryCall("ftell"); + } + result.offset = file_position; - if (fwrite(blob.data, 1, blob.size, context->swap_file) != blob.size) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } + if (fwrite(blob.data, 1, blob.size, context->swap_file) != blob.size) { + FailedLibraryCall("fwrite"); + } - if (fflush(context->swap_file) != 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } - } else { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; + if (fflush(context->swap_file) != 0) { + FailedLibraryCall("fflush"); } result.node_id = node_id; @@ -1624,21 +1625,14 @@ SwapBlob PutToSwap(SharedMemoryContext *context, RpcContext *rpc, size_t ReadFromSwap(SharedMemoryContext *context, Blob blob, SwapBlob swap_blob) { u32 node_id = swap_blob.node_id; - if (OpenSwapFile(context, node_id) == 0) { - if (fseek(context->swap_file, swap_blob.offset, SEEK_SET) != 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } - - if (fread(blob.data, 1, swap_blob.size, context->swap_file) != - swap_blob.size) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } + OpenSwapFile(context, node_id); + if (fseek(context->swap_file, swap_blob.offset, SEEK_SET) != 0) { + FailedLibraryCall("fseek"); + } - } else { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; + if (fread(blob.data, 1, swap_blob.size, context->swap_file) != + swap_blob.size) { + FailedLibraryCall("fread"); } return swap_blob.size; @@ -1707,13 +1701,11 @@ api::Status StdIoPersistBucket(SharedMemoryContext *context, RpcContext *rpc, // they were `Put`, but once we have a Trait that represents a file // mapping, we'll need pwrite and offsets. if (fwrite(data.data(), 1, num_bytes, file) != num_bytes) { - // TODO(chogan): @errorhandling result = STDIO_FWRITE_FAILED; LOG(ERROR) << result.Msg() << strerror(errno); break; } } else { - // TODO(chogan): @errorhandling result = READ_BLOB_FAILED; LOG(ERROR) << result.Msg(); break; @@ -1721,12 +1713,10 @@ api::Status StdIoPersistBucket(SharedMemoryContext *context, RpcContext *rpc, } if (fclose(file) != 0) { - // TODO(chogan): @errorhandling result = STDIO_FCLOSE_FAILED; LOG(ERROR) << result.Msg() << strerror(errno); } } else { - // TODO(chogan): @errorhandling result = STDIO_FOPEN_FAILED; LOG(ERROR) << result.Msg() << strerror(errno); } @@ -1755,22 +1745,20 @@ api::Status StdIoPersistBlob(SharedMemoryContext *context, RpcContext *rpc, LOG(INFO) << "STDIO Flush to file: " << " offset: " << offset << " of size:" << num_bytes << "." << std::endl; if (fwrite(data.data(), 1, num_bytes, file) != num_bytes) { - // TODO(chogan): @errorhandling result = STDIO_FWRITE_FAILED; LOG(ERROR) << result.Msg() << strerror(errno); } } else { - // TODO(chogan): @errorhandling result = STDIO_OFFSET_ERROR; LOG(ERROR) << result.Msg() << strerror(errno); } } } else { - // TODO(chogan): @errorhandling result = INVALID_FILE; LOG(ERROR) << result.Msg(); } + return result; } diff --git a/src/config_parser.cc b/src/config_parser.cc index e79fec1e6..d26658fcd 100644 --- a/src/config_parser.cc +++ b/src/config_parser.cc @@ -21,6 +21,7 @@ #include #include "hermes_types.h" +#include "utils.h" #include "memory_management.h" // Steps to add a new configuration variable: @@ -161,21 +162,17 @@ EntireFile ReadEntireFile(Arena *arena, const char *path) { } } else { - // TODO(chogan): @errorhandling - assert(!"ftell failed"); } } else { - // TODO(chogan): @errorhandling - assert(!"fseek failed"); + FailedLibraryCall("fseek"); } if (fclose(fstream) != 0) { - // TODO(chogan): @errorhandling + FailedLibraryCall("fclose"); } } else { - // TODO(chogan): @errorhandling - assert(!"fopen failed"); + FailedLibraryCall("fopen"); } return result; diff --git a/src/memory_management.cc b/src/memory_management.cc index f1e04afb3..2d421a1b9 100644 --- a/src/memory_management.cc +++ b/src/memory_management.cc @@ -85,21 +85,22 @@ size_t GetRemainingCapacity(Arena *arena) { return result; } +/** + * \brief The maximum capacity of \p arena becomes \p new_size + */ void GrowArena(Arena *arena, size_t new_size) { - if (new_size == 0) { - // TODO(chogan): @errorhandling - HERMES_NOT_IMPLEMENTED_YET; - } - - if (arena->capacity != new_size) { + if (new_size > arena->capacity) { void *new_base = (u8 *)realloc(arena->base, new_size); - if (new_base != arena->base) { + if (new_base) { arena->base = (u8 *)new_base; arena->capacity = new_size; } else { - // TODO(chogan): @errorhandling - assert(!"GrowArena failed\n"); + LOG(FATAL) << "realloc failed in " << __func__ << std::endl; } + } else { + LOG(WARNING) << __func__ << ": Not growing arena. " + << "Only accepts a size greater than the current arena " + << "capacity." << std::endl; } } @@ -394,7 +395,6 @@ u8 *HeapPushSize(Heap *heap, u32 size) { } EndTicketMutex(&heap->mutex); } else { - // TODO(chogan): @errorhandling heap->error_handler(); } } diff --git a/src/metadata_management.cc b/src/metadata_management.cc index e95cdb0ec..1a7b59484 100644 --- a/src/metadata_management.cc +++ b/src/metadata_management.cc @@ -719,7 +719,7 @@ void RenameBlob(SharedMemoryContext *context, RpcContext *rpc, DeleteBlobId(mdm, rpc, old_name, bucket_id); PutBlobId(mdm, rpc, new_name, blob_id, bucket_id); } else { - // TODO(chogan): @errorhandling + LOG(ERROR) << "Invalid BlobID for Blob '" << old_name << "'" << std::endl; } } diff --git a/src/utils.cc b/src/utils.cc index a441c77e6..8dedf4e23 100644 --- a/src/utils.cc +++ b/src/utils.cc @@ -114,6 +114,11 @@ void InitDefaultConfig(Config *config) { config->default_placement_policy = api::PlacementPolicy::kMinimizeIoTime; } +void FailedLibraryCall(std::string func) { + LOG(FATAL) << func << " failed with error: " << strerror(errno) + << std::endl; +} + namespace testing { TargetViewState InitDeviceState(u64 total_target, bool homo_dist) { diff --git a/src/utils.h b/src/utils.h index bc6a11338..12cc1af5d 100644 --- a/src/utils.h +++ b/src/utils.h @@ -94,6 +94,8 @@ size_t RoundDownToMultiple(size_t val, size_t multiple); */ void InitDefaultConfig(Config *config); +void FailedLibraryCall(std::string func); + namespace testing { enum class BlobSizeRange {