repaired some path handling, improved error handling

.gitignore

@@ -0,0 +1,2 @@
+.idea
+cmake-build-debug

include/ModuleLoader.hpp

@@ -2,25 +2,20 @@
 #include <string>
 #include <vector>
 #include "error.hpp"
-#include "dpm_interface.hpp"
 #include <filesystem>
 #include <dlfcn.h>
 #include <iostream>
-#include <module_interface.hpp>
+#include "module_interface.hpp"
-
-// Forward declaration to avoid circular dependency
-struct CommandArgs;
 
 class ModuleLoader {
 public:
     explicit ModuleLoader(std::string module_path = "/usr/lib/dpm/modules/");
     DPMError list_available_modules(std::vector<std::string>& 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;

include/dpm_interface.hpp

@@ -5,7 +5,8 @@
 #include "error.hpp"
 #include <dlfcn.h>
 #include <iomanip>
-#include "ModuleLoader.hpp"  // This should include ModuleLoader since it's used directly
+#include <filesystem>
+#include "ModuleLoader.hpp"
 
 /*
  *

src/ModuleLoader.cpp

@@ -2,11 +2,19 @@
 
 namespace fs = std::filesystem;
 
-ModuleLoader::ModuleLoader(std::string module_path) : module_path_(std::move(module_path))
+ModuleLoader::ModuleLoader(std::string 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_ += '/';
+        }
+    }
 }
 
 DPMError ModuleLoader::get_module_path(std::string& path) const
@@ -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<std::string>& modules) const
 {
     modules.clear();
 
     try {
-        fs::path absolute_path = fs::absolute(module_path_);
+        for (const auto& entry : fs::directory_iterator(module_path_)) {
-        for (const auto& entry : fs::directory_iterator(absolute_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) {
+    void* module_handle;
-        return DPMError::INVALID_MODULE;
+    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);
+    dlclose(module_handle);
-    return DPMError::SUCCESS;
+    return result;
 }
 
 DPMError ModuleLoader::get_module_version(void* module_handle, std::string& version) const

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
+    DPMError execute_error = loader.execute_module(args.module_name, args.command);
-    void * module_handle;
+    return print_error(execute_error, args.module_name, args.module_path);
-
-    // 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;
 }

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);
+    loader.get_module_path(path);
-    if (path_error != DPMError::SUCCESS) {
+
-        switch (path_error) {
+    if (!std::filesystem::exists(path)) {
-            case DPMError::PATH_NOT_FOUND:
+        std::cerr << "Module path does not exist: " << path << std::endl;
-                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;
-        }
         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<std::string> 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);
+        std::cerr << "No modules found in: " << path << std::endl;
-        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;
-        }
         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[])
 {