From 6317db11e55fc728ebab86e10e3995b394c1c96c Mon Sep 17 00:00:00 2001 From: Takeshi Yoneda Date: Thu, 20 Feb 2025 08:02:30 -0800 Subject: [PATCH] dynamic_modules: clean up duplicate call to filesystem::absolute (#38498) Commit Message: dynamic_modules: clean up duplicates call to filesystem::absolute Additional Description: Previously, during module loading phase, std::filesystem::absolute was called unnecessarily twice. This commit cleans up the code path and removes the duplicate call to it. Risk Level: low Testing: existing ones Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Takeshi Yoneda --- .../dynamic_modules/dynamic_modules.cc | 22 +++++++++---------- .../dynamic_modules/dynamic_modules.h | 7 +++--- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/source/extensions/dynamic_modules/dynamic_modules.cc b/source/extensions/dynamic_modules/dynamic_modules.cc index a0e5463d4945..e6a26b5f793b 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.cc +++ b/source/extensions/dynamic_modules/dynamic_modules.cc @@ -2,7 +2,6 @@ #include -#include #include #include "envoy/common/exception.h" @@ -16,9 +15,8 @@ namespace DynamicModules { constexpr char DYNAMIC_MODULES_SEARCH_PATH[] = "ENVOY_DYNAMIC_MODULES_SEARCH_PATH"; -absl::StatusOr newDynamicModule(const absl::string_view object_file_path, - const bool do_not_close) { - const std::filesystem::path file_path_absolute = std::filesystem::absolute(object_file_path); +absl::StatusOr +newDynamicModule(const std::filesystem::path& object_file_absolute_path, const bool do_not_close) { // From the man page of dlopen(3): // // > This can be used to test if the object is already resident (dlopen() returns NULL if it @@ -26,7 +24,7 @@ absl::StatusOr newDynamicModule(const absl::string_view object // // So we can use RTLD_NOLOAD to check if the module is already loaded to avoid the duplicate call // to the init function. - void* handle = dlopen(file_path_absolute.c_str(), RTLD_NOLOAD | RTLD_LAZY); + void* handle = dlopen(object_file_absolute_path.c_str(), RTLD_NOLOAD | RTLD_LAZY); if (handle != nullptr) { // This means the module is already loaded, and the return value is the handle of the already // loaded module. We don't need to call the init function again. @@ -39,10 +37,10 @@ absl::StatusOr newDynamicModule(const absl::string_view object if (do_not_close) { mode |= RTLD_NODELETE; } - handle = dlopen(file_path_absolute.c_str(), mode); + handle = dlopen(object_file_absolute_path.c_str(), mode); if (handle == nullptr) { - return absl::InvalidArgumentError( - absl::StrCat("Failed to load dynamic module: ", object_file_path, " : ", dlerror())); + return absl::InvalidArgumentError(absl::StrCat( + "Failed to load dynamic module: ", object_file_absolute_path.c_str(), " : ", dlerror())); } DynamicModulePtr dynamic_module = std::make_unique(handle); @@ -58,7 +56,7 @@ absl::StatusOr newDynamicModule(const absl::string_view object const char* abi_version = (*init_function.value())(); if (abi_version == nullptr) { return absl::InvalidArgumentError( - absl::StrCat("Failed to initialize dynamic module: ", object_file_path)); + absl::StrCat("Failed to initialize dynamic module: ", object_file_absolute_path.c_str())); } // Checks the kAbiVersion and the version of the dynamic module. if (absl::string_view(abi_version) != absl::string_view(kAbiVersion)) { @@ -76,9 +74,9 @@ absl::StatusOr newDynamicModuleByName(const absl::string_view " : ", DYNAMIC_MODULES_SEARCH_PATH, " is not set")); } - const std::filesystem::path file_path_absolute = std::filesystem::absolute( - fmt::format("{}/lib{}.so", std::string(module_search_path), std::string(module_name))); - return newDynamicModule(file_path_absolute.string(), do_not_close); + const std::filesystem::path file_path_absolute = + std::filesystem::absolute(fmt::format("{}/lib{}.so", module_search_path, module_name)); + return newDynamicModule(file_path_absolute, do_not_close); } DynamicModule::~DynamicModule() { dlclose(handle_); } diff --git a/source/extensions/dynamic_modules/dynamic_modules.h b/source/extensions/dynamic_modules/dynamic_modules.h index e0c222549741..1aa706d5f3e9 100644 --- a/source/extensions/dynamic_modules/dynamic_modules.h +++ b/source/extensions/dynamic_modules/dynamic_modules.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -57,14 +58,14 @@ using DynamicModulePtr = std::unique_ptr; /** * Creates a new DynamicModule. This is mainly exposed for testing purposes. Use * newDynamicModuleByName in wiring up dynamic modules. - * @param object_file_path the path to the object file to load. + * @param object_file_absolute_path the absolute path to the object file to load. * @param do_not_close if true, the dlopen will be called with RTLD_NODELETE, so the loaded object * will not be destroyed. This is useful when an object has some global state that should not be * terminated. For example, c-shared objects compiled by Go doesn't support dlclose * https://github.com/golang/go/issues/11100. */ -absl::StatusOr newDynamicModule(const absl::string_view object_file_path, - const bool do_not_close); +absl::StatusOr +newDynamicModule(const std::filesystem::path& object_file_absolute_path, const bool do_not_close); /** * Creates a new DynamicModule by name under the search path specified by the environment variable