Cleaning up initial commit and restructured some of the execution flow, fixed returns and reference mgmt

include/ModuleLoader.hpp

@@ -2,6 +2,11 @@
 #include <string>
 #include <vector>
 #include "error.hpp"
+#include "dpm_interface.hpp"
+#include <filesystem>
+#include <dlfcn.h>
+#include <iostream>
+#include <module_interface.hpp>
 
 // Forward declaration to avoid circular dependency
 struct CommandArgs;
@@ -9,14 +14,22 @@ struct CommandArgs;
 class ModuleLoader {
 public:
     explicit ModuleLoader(std::string module_path = "/usr/lib/dpm/modules/");
-    DPMError check_module_path() const;
+    DPMError list_available_modules(std::vector<std::string>& modules) const;
-    std::pair<std::vector<std::string>, DPMError> list_available_modules() const;
+    DPMError get_module_path(std::string& path) const;
-    const std::string& get_module_path() const { return module_path_; }
+    DPMError get_absolute_module_path(std::string& abs_path) const;
-    std::string get_absolute_module_path() const;
 
-    // Split into two separate methods
+    // Load and execute methods
-    void* load_module(const std::string& module_name) const;
+    DPMError load_module(const std::string& module_name, void*& module_handle) const;
-    int execute_module(void* module_handle, const std::string& command) const;
+    DPMError execute_module(void* module_handle, const std::string& command) const;
+
+    // Get module version
+    DPMError get_module_version(void* module_handle, std::string& version) const;
+
+    // Get module description
+    DPMError get_module_description(void* module_handle, std::string& description) const;
+
+    // Check if all required symbols from module_interface.hpp are exported by the module
+    DPMError validate_module_interface(void* module_handle, std::vector<std::string>& missing_symbols) const;
 
 private:
     std::string module_path_;

include/dpm_interface.hpp

@@ -3,6 +3,8 @@
 #include <getopt.h>
 #include <vector>
 #include "error.hpp"
+#include <dlfcn.h>
+#include <iomanip>
 #include "ModuleLoader.hpp"  // This should include ModuleLoader since it's used directly
 
 /*

include/module_interface.hpp

@@ -1,12 +1,29 @@
 #pragma once
 #include <string>
+#include <vector>
 
 /*
  *  Provides reserved symbol names we look for in modules.
  */
 
+// Define required symbols in one place
+namespace module_interface {
+    // This is the single source of truth for required module symbols
+    static const std::vector<std::string> required_symbols = {
+        "dpm_module_execute",
+        "dpm_module_get_version",
+        "dpm_get_description"
+    };
+}
+
 // Common interface for all DPM modules
 extern "C" {
     // Module must export this symbol to be considered valid
     int dpm_module_execute(const char* command, int argc, char** argv);
+
+    // Module version information
+    const char* dpm_module_get_version(void);
+
+    // Module description information
+    const char* dpm_get_description(void);
 }

modules/info.cpp

@@ -2,10 +2,22 @@
 #include <string>
 #include <cstring>
 #include <vector>
+#include "../include/module_interface.hpp"
+#include <gpgme.h>
 
 // Implementation of the info module
 // This module provides information about the DPM system
 
+// Version information
+extern "C" const char* dpm_module_get_version(void) {
+    return "0.1.0";
+}
+
+// Module description
+extern "C" const char* dpm_get_description(void) {
+    return "DPM Info Module - Provides information about the DPM system";
+}
+
 // Main entry point that will be called by DPM
 extern "C" int dpm_module_execute(const char* command, int argc, char** argv) {
     // Handle the case when no command is provided
@@ -30,7 +42,7 @@ extern "C" int dpm_module_execute(const char* command, int argc, char** argv) {
     }
     else if (cmd == "system") {
         std::cout << "System Information:\n";
-        std::cout << "  OS: " <<
+        std::cout << "  OS: "
 #ifdef _WIN32
             "Windows"
 #elif __APPLE__
@@ -41,7 +53,7 @@ extern "C" int dpm_module_execute(const char* command, int argc, char** argv) {
             "Unknown"
 #endif
             << "\n";
-        std::cout << "  Architecture: " <<
+        std::cout << "  Architecture: "
 #ifdef __x86_64__
             "x86_64"
 #elif __i386__

src/ModuleLoader.cpp

@@ -1,8 +1,4 @@
 #include "ModuleLoader.hpp"
-#include "dpm_interface.hpp"
-#include <filesystem>
-#include <dlfcn.h>
-#include <iostream>
 
 namespace fs = std::filesystem;
 
@@ -13,18 +9,37 @@ ModuleLoader::ModuleLoader(std::string module_path) : module_path_(std::move(mod
     }
 }
 
-DPMError ModuleLoader::check_module_path() const
+DPMError ModuleLoader::get_module_path(std::string& path) const
 {
-    if (!fs::exists(module_path_)) {
+    path = module_path_;
+    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;
     }
-
-    if (!fs::is_directory(module_path_)) {
-        return DPMError::PATH_NOT_DIRECTORY;
 }
 
+DPMError ModuleLoader::list_available_modules(std::vector<std::string>& modules) const
+{
+    modules.clear();
+
     try {
-        fs::directory_iterator(module_path_);
+        fs::path absolute_path = fs::absolute(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") {
+                    modules.push_back(filename.substr(0, filename.size() - 3));
+                }
+            }
+        }
     } catch (const fs::filesystem_error&) {
         return DPMError::PERMISSION_DENIED;
     }
@@ -32,75 +47,105 @@ DPMError ModuleLoader::check_module_path() const
     return DPMError::SUCCESS;
 }
 
-std::string ModuleLoader::get_absolute_module_path() const
+DPMError ModuleLoader::load_module(const std::string& module_name, void*& module_handle) const
 {
-    try {
-        return fs::absolute(module_path_).string();
-    } catch (const fs::filesystem_error&) {
-        return module_path_; // Return relative path if conversion fails
-    }
-}
-
-std::pair<std::vector<std::string>, DPMError> ModuleLoader::list_available_modules() const
-{
-    std::vector<std::string> modules;
-
-    try {
-        fs::path absolute_path = fs::absolute(module_path_);
-        for (const auto& entry : fs::directory_iterator(absolute_path)) {
-            if (entry.is_regular_file()) {
-                std::string filename = entry.path().filename().string();
-                // Check if it's a .so file
-                if (filename.size() > 3 && filename.substr(filename.size() - 3) == ".so") {
-                    // Remove the .so extension
-                    modules.push_back(filename.substr(0, filename.size() - 3));
-                }
-            }
-        }
-    } catch (const fs::filesystem_error&) {
-        return {modules, DPMError::PERMISSION_DENIED};
-    }
-
-    return {modules, DPMError::SUCCESS};
-}
-
-void* ModuleLoader::load_module(const std::string& module_name) const
-{
-    // Construct path to module shared object
     std::string module_so_path = module_path_ + module_name + ".so";
 
-    // Load the module
+    module_handle = dlopen(module_so_path.c_str(), RTLD_LAZY);
-    void* module_handle = dlopen(module_so_path.c_str(), RTLD_LAZY);
     if (!module_handle) {
-        std::cerr << "Failed to load module: " << dlerror() << std::endl;
+        return DPMError::MODULE_LOAD_FAILED;
-        return nullptr;
     }
 
-    // Clear any existing errors
     dlerror();
-
+    return DPMError::SUCCESS;
-    return module_handle;
 }
 
-int ModuleLoader::execute_module(void* module_handle, const std::string& command) const
+DPMError ModuleLoader::execute_module(void* module_handle, const std::string& command) const
 {
     if (!module_handle) {
-        std::cerr << "Invalid module handle" << std::endl;
+        return DPMError::INVALID_MODULE;
-        return 1;
     }
 
-    // Find the execution entry point
     using ExecuteFn = int (*)(const char*, int, char**);
     ExecuteFn execute_fn = (ExecuteFn)dlsym(module_handle, "dpm_module_execute");
 
     const char* error = dlerror();
     if (error != nullptr) {
-        std::cerr << "Failed to find module entry point: " << error << std::endl;
+        return DPMError::MODULE_LOAD_FAILED;
-        return 1;
     }
 
-    // Execute the module with just the provided command string
+    execute_fn(command.c_str(), 0, nullptr);
-    int result = execute_fn(command.c_str(), 0, nullptr);
+    return DPMError::SUCCESS;
-
+}
-    return result;
+
+DPMError ModuleLoader::get_module_version(void* module_handle, std::string& version) const
+{
+    if (!module_handle) {
+        version = "ERROR";
+        return DPMError::INVALID_MODULE;
+    }
+
+    dlerror();
+
+    using GetVersionFn = const char* (*)();
+    GetVersionFn get_version = (GetVersionFn)dlsym(module_handle, "dpm_module_get_version");
+
+    const char* error = dlerror();
+    if (error != nullptr) {
+        version = "unknown";
+        return DPMError::MODULE_LOAD_FAILED;
+    }
+
+    const char* ver = get_version();
+    version = ver ? ver : "unknown";
+    return DPMError::SUCCESS;
+}
+
+DPMError ModuleLoader::get_module_description(void* module_handle, std::string& description) const
+{
+    if (!module_handle) {
+        description = "ERROR";
+        return DPMError::INVALID_MODULE;
+    }
+
+    dlerror();
+
+    using GetDescriptionFn = const char* (*)();
+    GetDescriptionFn get_description = (GetDescriptionFn)dlsym(module_handle, "dpm_get_description");
+
+    const char* error = dlerror();
+    if (error != nullptr) {
+        description = "unknown";
+        return DPMError::MODULE_LOAD_FAILED;
+    }
+
+    const char* desc = get_description();
+    description = desc ? desc : "unknown";
+    return DPMError::SUCCESS;
+}
+
+DPMError ModuleLoader::validate_module_interface(void* module_handle, std::vector<std::string>& missing_symbols) const
+{
+    if (!module_handle) {
+        return DPMError::INVALID_MODULE;
+    }
+
+    missing_symbols.clear();
+
+    size_t num_symbols = module_interface::required_symbols.size();
+    for (size_t i = 0; i < num_symbols; i++) {
+        dlerror();
+        void* sym = dlsym(module_handle, module_interface::required_symbols[i].c_str());
+        const char* error = dlerror();
+
+        if (error != nullptr) {
+            missing_symbols.push_back(module_interface::required_symbols[i]);
+        }
+    }
+
+    if (missing_symbols.empty()) {
+        return DPMError::SUCCESS;
+    }
+
+    return DPMError::INVALID_MODULE;
 }

src/dpm.cpp

@@ -20,31 +20,56 @@ int default_behavior(const ModuleLoader& loader)
 // entry point for the DPM utility
 int main( int argc, char* argv[] )
 {
+    // process the arguments suppplied to DPM and provide
+    // an object that contains them for command and routing
+    // processing
     auto args = parse_args( argc, argv );
+
+    // create a module loader object at the supplied or default path
+    // TODO: the default is set in the header instead of the
+    //   implementation, fix that
     ModuleLoader loader( args.module_path );
 
-    // check if the modules path even exists and return an error if not since we can't do anything
+    // check the module path for the loader object
-    if (auto result = main_check_module_path(loader); result != 0) {
+    int path_check_result = main_check_module_path( loader );
-        return result;
+    if ( path_check_result != 0 ) {
+        // exit if there's an error and ensure
+        // it has an appropriate return code
+        return 1;
     }
 
-    // if no modules are supplied, execute the default behaviour and exit
+    // if no module is provided to execute, then trigger the default
+    // dpm behaviour
     if ( args.module_name.empty() ) {
         return default_behavior( loader );
     }
 
-    // load the module specified
+    // create a module handle
-    void* module_handle = loader.load_module(args.module_name);
+    void * module_handle;
-    if (!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 with the command string
+    // execute the module and provide the user-supplied command to execute
-    int result = loader.execute_module(module_handle, args.command);
+    DPMError execute_error = loader.execute_module( module_handle, args.command );
 
-    // Cleanup
+    // there is no retry logic, so, whether execute succeeded
+    // or failed, clean up the module handle
     dlclose(module_handle);
 
-    return result;
+    // 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

@@ -11,47 +11,123 @@
 // check if the module path exists
 int main_check_module_path(const ModuleLoader& loader)
 {
-    if (auto result = loader.check_module_path(); result != DPMError::SUCCESS) {
+    std::string path;
-        switch (result) {
+    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: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Module path not found: " << path << std::endl;
             break;
             case DPMError::PATH_NOT_DIRECTORY:
-                std::cerr << "Not a directory: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Not a directory: " << path << std::endl;
             break;
             case DPMError::PERMISSION_DENIED:
-                std::cerr << "Permission denied: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Permission denied: " << path << std::endl;
             break;
             default:
-                std::cerr << "Failed checking module path: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Failed checking module path: " << path << std::endl;
         }
         return 1;
     }
     return 0;
 }
 
-// list the modules
+// list the modules with version information in table format
 int main_list_modules(const ModuleLoader& loader)
 {
-    auto [modules, list_error] = loader.list_available_modules();
+    std::vector<std::string> modules;
+    std::string path, abs_path;
+
+    DPMError get_path_error = loader.get_module_path(path);
+    if (get_path_error != DPMError::SUCCESS) {
+        std::cerr << "Failed to get module path" << std::endl;
+        return 1;
+    }
+
+    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: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Permission denied reading modules from: " << path << std::endl;
             break;
             default:
-                std::cerr << "Failed listing modules from: " << loader.get_absolute_module_path() << std::endl;
+                std::cerr << "Failed listing modules from: " << path << std::endl;
         }
         return 1;
     }
 
-    std::cout << "Available modules in " << loader.get_absolute_module_path() << ":\n";
+    if (modules.empty()) {
-    for (const auto& module : modules) {
+        std::cout << "No modules found in '" << path << "'." << std::endl;
-        std::cout << "  " << module << "\n";
-    }
         return 0;
     }
 
+    std::vector<std::string> valid_modules;
+    for (const auto& module : modules) {
+        void* handle;
+        DPMError load_error = loader.load_module(module, handle);
+        if (load_error != DPMError::SUCCESS) {
+            continue;
+        }
+
+        std::vector<std::string> missing_symbols;
+        DPMError validate_error = loader.validate_module_interface(handle, missing_symbols);
+        if (validate_error == DPMError::SUCCESS) {
+            valid_modules.push_back(module);
+        }
+        dlclose(handle);
+    }
+
+    if (valid_modules.empty()) {
+        std::cout << "No valid modules found in '" << path << "'." << std::endl;
+        return 0;
+    }
+
+    size_t max_name_length = 0;
+    size_t max_version_length = 0;
+    for (const auto& module : valid_modules) {
+        void* module_handle;
+        std::string version;
+        max_name_length = std::max(max_name_length, module.length());
+
+        DPMError load_error = loader.load_module(module, module_handle);
+        if (load_error == DPMError::SUCCESS) {
+            DPMError version_error = loader.get_module_version(module_handle, version);
+            if (version_error == DPMError::SUCCESS) {
+                max_version_length = std::max(max_version_length, version.length());
+            }
+            dlclose(module_handle);
+        }
+    }
+
+    const int column_spacing = 4;
+
+    std::cout << "Available modules in '" << path << "':" << std::endl << std::endl;
+    std::cout << std::left << std::setw(max_name_length + column_spacing) << "MODULE"
+              << std::setw(max_version_length + column_spacing) << "VERSION"
+              << "DESCRIPTION" << std::endl;
+
+    for (const auto& module_name : valid_modules) {
+        void* module_handle;
+        std::string version = "unknown";
+        std::string description = "unknown";
+
+        DPMError load_error = loader.load_module(module_name, module_handle);
+        if (load_error == DPMError::SUCCESS) {
+            DPMError version_error = loader.get_module_version(module_handle, version);
+            DPMError desc_error = loader.get_module_description(module_handle, description);
+            dlclose(module_handle);
+        }
+
+        std::cout << std::left << std::setw(max_name_length + column_spacing) << module_name
+                  << std::setw(max_version_length + column_spacing) << version
+                  << description << std::endl;
+    }
+
+    return 0;
+}
+
+
 // parser for populating data structure for supplied arguments
 CommandArgs parse_args(int argc, char* argv[])
 {
@@ -71,28 +147,25 @@ CommandArgs parse_args(int argc, char* argv[])
                 args.module_path = optarg;
             break;
             case 'h':
-                std::cout << "Usage: dpm [options] [module-name] [module args...]\n"
+                std::cout << "Usage: dpm [options] [module-name] [module args...]\n\n"
-                         << "Options:\n"
+                         << "Options:\n\n"
                          << "  -m, --module-path PATH   Path to DPM modules\n"
                          << "  -h, --help              Show this help message\n"
-                         << "\nIf no module is specified, available modules will be listed.\n";
+                         << "\nIf no module is specified, available modules will be listed.\n\n";
             exit(0);
             case '?':
                 exit(1);
         }
     }
 
-    // If there are remaining args, the first one is the module name
     if (optind < argc) {
         args.module_name = argv[optind++];
 
-        // Collect all remaining arguments and combine them into a single command string
         for (int i = optind; i < argc; i++) {
             if (!args.command.empty()) {
                 args.command += " ";
             }
 
-            // Handle arguments with spaces by quoting them
             std::string arg = argv[i];
             if (arg.find(' ') != std::string::npos) {
                 args.command += "\"" + arg + "\"";