From 2ec8908fa91b93d35c49d04b14c7196438018abd Mon Sep 17 00:00:00 2001 From: Chris Punches Date: Sun, 23 Feb 2025 04:04:50 -0500 Subject: [PATCH] repaired some path handling, improved error handling --- .gitignore | 2 ++ include/ModuleLoader.hpp | 9 ++---- include/dpm_interface.hpp | 3 +- src/ModuleLoader.cpp | 47 ++++++++++++++++--------------- src/dpm.cpp | 59 ++++++++++++++++++++------------------- src/dpm_interface.cpp | 50 ++++++++++++++++----------------- 6 files changed, 86 insertions(+), 84 deletions(-) create mode 100644 .gitignore diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..8e24b65 --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +.idea +cmake-build-debug diff --git a/include/ModuleLoader.hpp b/include/ModuleLoader.hpp index 687ce99..fa140cd 100644 --- a/include/ModuleLoader.hpp +++ b/include/ModuleLoader.hpp @@ -2,25 +2,20 @@ #include #include #include "error.hpp" -#include "dpm_interface.hpp" #include #include #include -#include - -// Forward declaration to avoid circular dependency -struct CommandArgs; +#include "module_interface.hpp" class ModuleLoader { public: explicit ModuleLoader(std::string module_path = "/usr/lib/dpm/modules/"); DPMError list_available_modules(std::vector& modules) const; DPMError get_module_path(std::string& path) const; - DPMError get_absolute_module_path(std::string& abs_path) const; // Load and execute methods DPMError load_module(const std::string& module_name, void*& module_handle) const; - DPMError execute_module(void* module_handle, const std::string& command) const; + DPMError execute_module(const std::string& module_name, const std::string& command) const; // Get module version DPMError get_module_version(void* module_handle, std::string& version) const; diff --git a/include/dpm_interface.hpp b/include/dpm_interface.hpp index abba3a2..c72f593 100644 --- a/include/dpm_interface.hpp +++ b/include/dpm_interface.hpp @@ -5,7 +5,8 @@ #include "error.hpp" #include #include -#include "ModuleLoader.hpp" // This should include ModuleLoader since it's used directly +#include +#include "ModuleLoader.hpp" /* * diff --git a/src/ModuleLoader.cpp b/src/ModuleLoader.cpp index 18dd92e..2dee215 100644 --- a/src/ModuleLoader.cpp +++ b/src/ModuleLoader.cpp @@ -2,10 +2,18 @@ namespace fs = std::filesystem; -ModuleLoader::ModuleLoader(std::string module_path) : module_path_(std::move(module_path)) +ModuleLoader::ModuleLoader(std::string module_path) { - if (!module_path_.empty() && module_path_.back() != '/') { - module_path_ += '/'; + try { + module_path_ = fs::absolute(module_path).string(); + if (!module_path_.empty() && module_path_.back() != '/') { + module_path_ += '/'; + } + } catch (const fs::filesystem_error&) { + module_path_ = module_path; + if (!module_path_.empty() && module_path_.back() != '/') { + module_path_ += '/'; + } } } @@ -15,24 +23,12 @@ DPMError ModuleLoader::get_module_path(std::string& path) const return DPMError::SUCCESS; } -DPMError ModuleLoader::get_absolute_module_path(std::string& abs_path) const -{ - try { - abs_path = fs::absolute(module_path_).string(); - return DPMError::SUCCESS; - } catch (const fs::filesystem_error&) { - abs_path = module_path_; - return DPMError::PATH_NOT_FOUND; - } -} - DPMError ModuleLoader::list_available_modules(std::vector& modules) const { modules.clear(); try { - fs::path absolute_path = fs::absolute(module_path_); - for (const auto& entry : fs::directory_iterator(absolute_path)) { + for (const auto& entry : fs::directory_iterator(module_path_)) { if (entry.is_regular_file()) { std::string filename = entry.path().filename().string(); if (filename.size() > 3 && filename.substr(filename.size() - 3) == ".so") { @@ -60,22 +56,29 @@ DPMError ModuleLoader::load_module(const std::string& module_name, void*& module return DPMError::SUCCESS; } -DPMError ModuleLoader::execute_module(void* module_handle, const std::string& command) const +DPMError ModuleLoader::execute_module(const std::string& module_name, const std::string& command) const { - if (!module_handle) { - return DPMError::INVALID_MODULE; + void* module_handle; + DPMError load_error = load_module(module_name, module_handle); + + if (load_error != DPMError::SUCCESS) { + return load_error; } using ExecuteFn = int (*)(const char*, int, char**); ExecuteFn execute_fn = (ExecuteFn)dlsym(module_handle, "dpm_module_execute"); const char* error = dlerror(); + DPMError result = DPMError::SUCCESS; + if (error != nullptr) { - return DPMError::MODULE_LOAD_FAILED; + result = DPMError::MODULE_LOAD_FAILED; + } else { + execute_fn(command.c_str(), 0, nullptr); } - execute_fn(command.c_str(), 0, nullptr); - return DPMError::SUCCESS; + dlclose(module_handle); + return result; } DPMError ModuleLoader::get_module_version(void* module_handle, std::string& version) const diff --git a/src/dpm.cpp b/src/dpm.cpp index 6be1bc2..1a357c6 100644 --- a/src/dpm.cpp +++ b/src/dpm.cpp @@ -11,6 +11,35 @@ * 3. Provide a module-agnostic unified interface for modules. */ +// prints error message and returns error code +int print_error(DPMError error, const std::string& module_name, const std::string& module_path) { + switch (error) { + case DPMError::SUCCESS: + return 0; + case DPMError::PATH_NOT_FOUND: + std::cerr << "Module path not found: " << module_path << std::endl; + return 1; + case DPMError::PATH_NOT_DIRECTORY: + std::cerr << "Module path is not a directory: " << module_path << std::endl; + return 1; + case DPMError::PERMISSION_DENIED: + std::cerr << "Permission denied accessing module: " << module_name << std::endl; + return 1; + case DPMError::MODULE_NOT_FOUND: + std::cerr << "Module not found: " << module_name << std::endl; + return 1; + case DPMError::MODULE_LOAD_FAILED: + std::cerr << "Failed to load module: " << module_name << std::endl; + return 1; + case DPMError::INVALID_MODULE: + std::cerr << "Invalid module format: " << module_name << std::endl; + return 1; + default: + std::cerr << "Unknown error executing module: " << module_name << std::endl; + return 1; + } +} + // the default behaviour if dpm is executed without being told to do anything int default_behavior(const ModuleLoader& loader) { @@ -44,32 +73,6 @@ int main( int argc, char* argv[] ) return default_behavior( loader ); } - // create a module handle - void * module_handle; - - // load the user-supplied module to execute - DPMError load_error = loader.load_module( args.module_name, module_handle ); - - // if that failed, additionally print an error and return a non-zero exit code - // TODO: verify that loader.load_module is actually doing error handling - if ( load_error != DPMError::SUCCESS ) { - std::cerr << "Failed to load module: " << args.module_name << std::endl; - return 1; - } - - // execute the module and provide the user-supplied command to execute - DPMError execute_error = loader.execute_module( module_handle, args.command ); - - // there is no retry logic, so, whether execute succeeded - // or failed, clean up the module handle - dlclose(module_handle); - - // check the execution result and if it failed, report an additional error - // TODO: verify that loader.execute_module is actually doing error handling - if (execute_error != DPMError::SUCCESS) { - std::cerr << "Failed to execute module: " << args.module_name << std::endl; - return 1; - } - - return 0; + DPMError execute_error = loader.execute_module(args.module_name, args.command); + return print_error(execute_error, args.module_name, args.module_path); } \ No newline at end of file diff --git a/src/dpm_interface.cpp b/src/dpm_interface.cpp index 15b7f27..2e64abf 100644 --- a/src/dpm_interface.cpp +++ b/src/dpm_interface.cpp @@ -12,31 +12,37 @@ int main_check_module_path(const ModuleLoader& loader) { std::string path; - DPMError path_error = loader.get_absolute_module_path(path); - if (path_error != DPMError::SUCCESS) { - switch (path_error) { - case DPMError::PATH_NOT_FOUND: - std::cerr << "Module path not found: " << path << std::endl; - break; - case DPMError::PATH_NOT_DIRECTORY: - std::cerr << "Not a directory: " << path << std::endl; - break; - case DPMError::PERMISSION_DENIED: - std::cerr << "Permission denied: " << path << std::endl; - break; - default: - std::cerr << "Failed checking module path: " << path << std::endl; - } + loader.get_module_path(path); + + if (!std::filesystem::exists(path)) { + std::cerr << "Module path does not exist: " << path << std::endl; return 1; } + + if (!std::filesystem::is_directory(path)) { + std::cerr << "Module path is not a directory: " << path << std::endl; + return 1; + } + + try { + auto perms = std::filesystem::status(path).permissions(); + if ((perms & std::filesystem::perms::owner_read) == std::filesystem::perms::none) { + std::cerr << "Permission denied: " << path << std::endl; + return 1; + } + } catch (const std::filesystem::filesystem_error&) { + std::cerr << "Permission denied: " << path << std::endl; + return 1; + } + return 0; } -// list the modules with version information in table format +// list the modules int main_list_modules(const ModuleLoader& loader) { std::vector modules; - std::string path, abs_path; + std::string path; DPMError get_path_error = loader.get_module_path(path); if (get_path_error != DPMError::SUCCESS) { @@ -46,14 +52,7 @@ int main_list_modules(const ModuleLoader& loader) DPMError list_error = loader.list_available_modules(modules); if (list_error != DPMError::SUCCESS) { - loader.get_absolute_module_path(abs_path); - switch (list_error) { - case DPMError::PERMISSION_DENIED: - std::cerr << "Permission denied reading modules from: " << path << std::endl; - break; - default: - std::cerr << "Failed listing modules from: " << path << std::endl; - } + std::cerr << "No modules found in: " << path << std::endl; return 1; } @@ -127,7 +126,6 @@ int main_list_modules(const ModuleLoader& loader) return 0; } - // parser for populating data structure for supplied arguments CommandArgs parse_args(int argc, char* argv[]) {