From 7573e99f46e959ef5c7fca09b497ca45b5471d0a Mon Sep 17 00:00:00 2001 From: Jimmy Ye Date: Wed, 21 Aug 2024 14:10:23 -0700 Subject: [PATCH] Move out utility functions from platform_manager to helpers Summary: Move out `createDirectories` and `getStringFileContent` from `fboss::platform::platform_manager::Utils` to `fboss::platform::PlatformUtils` (under `platform/helpers/` directory) This allows re-use outside of Platform Manager; these utility functions are not tied to PM-specific logic. In particular this is used by following diff which adds a caching feature to `PlatformNameLib` that saves the results of `dmidecode` to a file. Without this refactor, there would be a circular dependency since `Utils::getConfig` calls `PlatformNameLib`, while `PlatformNameLib` would call `Utils::createDirectories`/`Utils::getStringFileContent`. Reviewed By: Scott8440 Differential Revision: D61427074 fbshipit-source-id: 8ecda71c18e64d6c26c94e0046a19c8bf87f394f --- fboss/platform/helpers/MockPlatformUtils.h | 5 ++++ fboss/platform/helpers/PlatformUtils.cpp | 22 ++++++++++++++ fboss/platform/helpers/PlatformUtils.h | 10 +++++++ fboss/platform/helpers/tests/BUCK | 11 +++++++ .../helpers/tests/PlatformUtilsTest.cpp | 30 +++++++++++++++++++ fboss/platform/platform_manager/BUCK | 2 ++ .../platform_manager/PlatformExplorer.cpp | 6 ++-- .../platform_manager/PresenceChecker.cpp | 11 ++++--- .../platform_manager/PresenceChecker.h | 6 +++- fboss/platform/platform_manager/Utils.cpp | 19 ------------ fboss/platform/platform_manager/Utils.h | 8 ----- fboss/platform/platform_manager/tests/BUCK | 4 +-- .../tests/PlatformExplorerTest.cpp | 4 +-- .../tests/PresenceCheckerTest.cpp | 17 +++++------ .../platform_manager/tests/UtilsTest.cpp | 16 ---------- 15 files changed, 107 insertions(+), 64 deletions(-) create mode 100644 fboss/platform/helpers/tests/PlatformUtilsTest.cpp diff --git a/fboss/platform/helpers/MockPlatformUtils.h b/fboss/platform/helpers/MockPlatformUtils.h index e247d85fb7c7b..922929d91882f 100644 --- a/fboss/platform/helpers/MockPlatformUtils.h +++ b/fboss/platform/helpers/MockPlatformUtils.h @@ -14,5 +14,10 @@ class MockPlatformUtils : public PlatformUtils { execCommand, (const std::string&), (const)); + MOCK_METHOD( + (std::optional), + getStringFileContent, + (const std::string&), + (const)); }; } // namespace facebook::fboss::platform diff --git a/fboss/platform/helpers/PlatformUtils.cpp b/fboss/platform/helpers/PlatformUtils.cpp index 4842e13761753..207e28358cdf2 100644 --- a/fboss/platform/helpers/PlatformUtils.cpp +++ b/fboss/platform/helpers/PlatformUtils.cpp @@ -2,10 +2,13 @@ #include "fboss/platform/helpers/PlatformUtils.h" +#include + #include #include #include +namespace fs = std::filesystem; using namespace folly::literals::shell_literals; namespace facebook::fboss::platform { @@ -20,4 +23,23 @@ std::pair PlatformUtils::execCommand( return std::make_pair(exitStatus, standardOut); } +bool PlatformUtils::createDirectories(const std::string& path) { + std::error_code errCode; + std::filesystem::create_directories(fs::path(path), errCode); + if (errCode.value() != 0) { + XLOG(ERR) << fmt::format( + "Received error code {} from creating path {}", errCode.value(), path); + } + return errCode.value() == 0; +} + +std::optional PlatformUtils::getStringFileContent( + const std::string& path) const { + std::string value{}; + if (!folly::readFile(path.c_str(), value)) { + return std::nullopt; + } + return folly::trimWhitespace(value).str(); +} + } // namespace facebook::fboss::platform diff --git a/fboss/platform/helpers/PlatformUtils.h b/fboss/platform/helpers/PlatformUtils.h index 43b446ec9ce1e..2dabd323e7316 100644 --- a/fboss/platform/helpers/PlatformUtils.h +++ b/fboss/platform/helpers/PlatformUtils.h @@ -2,6 +2,7 @@ #pragma once +#include #include namespace facebook::fboss::platform { @@ -11,6 +12,15 @@ class PlatformUtils { virtual ~PlatformUtils() = default; // Executes command and returns exit status and standard output virtual std::pair execCommand(const std::string& cmd) const; + + // Recursively create directories for the given path. + // - for given /x/y/z, directory y/z if x already exists. + // No-op if parent directories already exist. + // Returns true if created or already exist, otherwise false. + bool createDirectories(const std::string& path); + + virtual std::optional getStringFileContent( + const std::string& path) const; }; } // namespace facebook::fboss::platform diff --git a/fboss/platform/helpers/tests/BUCK b/fboss/platform/helpers/tests/BUCK index 70784fa19209d..da7f135de911e 100644 --- a/fboss/platform/helpers/tests/BUCK +++ b/fboss/platform/helpers/tests/BUCK @@ -13,3 +13,14 @@ cpp_unittest( "//fboss/platform/helpers:platform_name_lib", ], ) + +cpp_unittest( + name = "platform_utils_test", + srcs = [ + "PlatformUtilsTest.cpp", + ], + deps = [ + "//fboss/platform/helpers:platform_utils", + "//folly/testing:test_util", + ], +) diff --git a/fboss/platform/helpers/tests/PlatformUtilsTest.cpp b/fboss/platform/helpers/tests/PlatformUtilsTest.cpp new file mode 100644 index 0000000000000..0442110908739 --- /dev/null +++ b/fboss/platform/helpers/tests/PlatformUtilsTest.cpp @@ -0,0 +1,30 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include + +#include +#include + +#include "fboss/platform/helpers/PlatformUtils.h" + +namespace fs = std::filesystem; +using namespace ::testing; +using namespace facebook::fboss::platform; + +namespace facebook::fboss::platform { + +TEST(PlatformUtilsTest, CreateParentDirectories) { + auto tmpPath = folly::test::TemporaryDirectory().path(); + EXPECT_TRUE(PlatformUtils().createDirectories(tmpPath.string())); + EXPECT_TRUE(fs::exists(tmpPath.string())); + tmpPath = tmpPath / "a"; + EXPECT_TRUE(PlatformUtils().createDirectories(tmpPath.string())); + EXPECT_TRUE(fs::exists(tmpPath.string())); + tmpPath = tmpPath / "b" / "c"; + EXPECT_TRUE(PlatformUtils().createDirectories(tmpPath.string())); + EXPECT_TRUE(fs::exists(tmpPath.string())); + EXPECT_TRUE(PlatformUtils().createDirectories("/")); + EXPECT_FALSE(PlatformUtils().createDirectories("")); +} + +} // namespace facebook::fboss::platform diff --git a/fboss/platform/platform_manager/BUCK b/fboss/platform/platform_manager/BUCK index 0fe184bbbcc5b..806ff994001d9 100644 --- a/fboss/platform/platform_manager/BUCK +++ b/fboss/platform/platform_manager/BUCK @@ -109,6 +109,7 @@ cpp_library( exported_deps = [ ":device_path_resolver", ":utils", + "//fboss/platform/helpers:platform_utils", ], ) @@ -140,6 +141,7 @@ cpp_library( ":presence_checker", ":utils", "//fb303:service_data", + "//fboss/platform/helpers:platform_utils", "//fboss/platform/weutil:fboss_eeprom_lib", "//fboss/platform/weutil:ioctl_smbus_eeprom_reader", "//folly:file_util", diff --git a/fboss/platform/platform_manager/PlatformExplorer.cpp b/fboss/platform/platform_manager/PlatformExplorer.cpp index 2b93d4e0f8231..40c88339cb86b 100644 --- a/fboss/platform/platform_manager/PlatformExplorer.cpp +++ b/fboss/platform/platform_manager/PlatformExplorer.cpp @@ -14,6 +14,7 @@ #include #include +#include "fboss/platform/helpers/PlatformUtils.h" #include "fboss/platform/platform_manager/Utils.h" #include "fboss/platform/platform_manager/gen-cpp2/platform_manager_config_constants.h" @@ -38,8 +39,7 @@ std::string getSlotPath( // TODO: Handle hwmon/info_rom cases (by standardizing them away, if possible). int readVersionNumber(const std::string& path) { const auto versionFileContent = - facebook::fboss::platform ::platform_manager::Utils() - .getStringFileContent(path); + facebook::fboss::platform::PlatformUtils().getStringFileContent(path); if (!versionFileContent) { // This log is necessary to distinguish read error vs reading "0". XLOGF( @@ -494,7 +494,7 @@ void PlatformExplorer::createDeviceSymLink( const std::string& linkPath, const std::string& devicePath) { auto linkParentPath = std::filesystem::path(linkPath).parent_path(); - if (!Utils().createDirectories(linkParentPath.string())) { + if (!PlatformUtils().createDirectories(linkParentPath.string())) { XLOG(ERR) << fmt::format( "Failed to create the parent path ({})", linkParentPath.string()); return; diff --git a/fboss/platform/platform_manager/PresenceChecker.cpp b/fboss/platform/platform_manager/PresenceChecker.cpp index 37774a9f73981..894c3a4eac4a8 100644 --- a/fboss/platform/platform_manager/PresenceChecker.cpp +++ b/fboss/platform/platform_manager/PresenceChecker.cpp @@ -1,14 +1,16 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. #include "fboss/platform/platform_manager/PresenceChecker.h" -#include "fboss/platform/platform_manager/Utils.h" using namespace facebook::fboss::platform::platform_manager; PresenceChecker::PresenceChecker( const DevicePathResolver& devicePathResolver, - const std::shared_ptr utils) - : devicePathResolver_(devicePathResolver), utils_(utils) {} + const std::shared_ptr utils, + const std::shared_ptr platformUtils) + : devicePathResolver_(devicePathResolver), + utils_(utils), + platformUtils_(platformUtils) {} bool PresenceChecker::isPresent( const PresenceDetection& presenceDetection, @@ -39,7 +41,8 @@ bool PresenceChecker::sysfsPresent( *handle.presenceFileName(), *handle.devicePath(), *presencePath); - auto presenceFileContent = utils_->getStringFileContent(*presencePath); + auto presenceFileContent = + platformUtils_->getStringFileContent(*presencePath); if (!presenceFileContent) { throw std::runtime_error( fmt::format("Could not read file {}", *presencePath)); diff --git a/fboss/platform/platform_manager/PresenceChecker.h b/fboss/platform/platform_manager/PresenceChecker.h index e47f9219b834c..ec23a6f0f9d63 100644 --- a/fboss/platform/platform_manager/PresenceChecker.h +++ b/fboss/platform/platform_manager/PresenceChecker.h @@ -2,6 +2,7 @@ #pragma once +#include "fboss/platform/helpers/PlatformUtils.h" #include "fboss/platform/platform_manager/DevicePathResolver.h" #include "fboss/platform/platform_manager/Utils.h" @@ -11,7 +12,9 @@ class PresenceChecker { public: explicit PresenceChecker( const DevicePathResolver& devicePathResolver, - const std::shared_ptr utils = std::make_shared()); + const std::shared_ptr utils = std::make_shared(), + const std::shared_ptr platformUtils = + std::make_shared()); bool isPresent( const PresenceDetection& presenceDetection, @@ -20,6 +23,7 @@ class PresenceChecker { private: const DevicePathResolver& devicePathResolver_; const std::shared_ptr utils_; + const std::shared_ptr platformUtils_; bool sysfsPresent(const SysfsFileHandle& handle, const std::string& slotPath); bool gpioPresent(const GpioLineHandle& handle, const std::string& slotPath); diff --git a/fboss/platform/platform_manager/Utils.cpp b/fboss/platform/platform_manager/Utils.cpp index 8b4c3ae69a8d5..be96de768a43e 100644 --- a/fboss/platform/platform_manager/Utils.cpp +++ b/fboss/platform/platform_manager/Utils.cpp @@ -88,16 +88,6 @@ PlatformConfig Utils::getConfig() { return config; } -bool Utils::createDirectories(const std::string& path) { - std::error_code errCode; - std::filesystem::create_directories(fs::path(path), errCode); - if (errCode.value() != 0) { - XLOG(ERR) << fmt::format( - "Received error code {} from creating path {}", errCode.value(), path); - } - return errCode.value() == 0; -} - std::pair Utils::parseDevicePath( const std::string& devicePath) { if (!ConfigValidator().isValidDevicePath(devicePath)) { @@ -180,15 +170,6 @@ std::string Utils::resolveWatchdogCharDevPath(const std::string& sysfsPath) { return charDevPath; } -std::optional Utils::getStringFileContent( - const std::string& path) const { - std::string value{}; - if (!folly::readFile(path.c_str(), value)) { - return std::nullopt; - } - return folly::trimWhitespace(value).str(); -} - int Utils::getGpioLineValue(const std::string& charDevPath, int lineIndex) const { struct gpiod_chip* chip = gpiod_chip_open(charDevPath.c_str()); diff --git a/fboss/platform/platform_manager/Utils.h b/fboss/platform/platform_manager/Utils.h index da198e8454e42..52b9e931c2c7f 100644 --- a/fboss/platform/platform_manager/Utils.h +++ b/fboss/platform/platform_manager/Utils.h @@ -10,11 +10,6 @@ class Utils { public: virtual ~Utils() = default; PlatformConfig getConfig(); - // Recursively create directories for the given path. - // - for given /x/y/z, directory y/z if x already exists. - // No-op if parent directories already exist. - // Returns true if created or already exist, otherwise false. - bool createDirectories(const std::string& path); // Extract (SlotPath, DeviceName) from DevicePath. // Returns a pair of (SlotPath, DeviceName). Throws if DevicePath is invalid. @@ -37,9 +32,6 @@ class Utils { // Throws an exception when it fails to resolve CharDevicePath std::string resolveWatchdogCharDevPath(const std::string& sysfsPath); - virtual std::optional getStringFileContent( - const std::string& path) const; - virtual int getGpioLineValue(const std::string& charDevPath, int lineIndex) const; }; diff --git a/fboss/platform/platform_manager/tests/BUCK b/fboss/platform/platform_manager/tests/BUCK index fd43e91d799e7..c712497b4a467 100644 --- a/fboss/platform/platform_manager/tests/BUCK +++ b/fboss/platform/platform_manager/tests/BUCK @@ -32,8 +32,8 @@ cpp_unittest( ], deps = [ "//fb303:service_data", + "//fboss/platform/helpers:platform_utils", "//fboss/platform/platform_manager:platform_explorer", - "//fboss/platform/platform_manager:utils", "//folly:file_util", "//folly/testing:test_util", ], @@ -46,7 +46,6 @@ cpp_unittest( ], deps = [ "//fboss/platform/platform_manager:utils", - "//folly/testing:test_util", ], ) @@ -78,6 +77,7 @@ cpp_unittest( ], deps = [ "fbsource//third-party/googletest:gmock", + "//fboss/platform/helpers:mock_platform_utils", "//fboss/platform/platform_manager:presence_checker", "//fboss/platform/platform_manager:utils", ], diff --git a/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp b/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp index f87cf64da4d43..06477189bda32 100644 --- a/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp +++ b/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp @@ -5,8 +5,8 @@ #include #include +#include "fboss/platform/helpers/PlatformUtils.h" #include "fboss/platform/platform_manager/PlatformExplorer.h" -#include "fboss/platform/platform_manager/Utils.h" using namespace ::testing; using namespace facebook::fboss::platform; @@ -18,7 +18,7 @@ void writeVersions( std::string deviceType, const char* version, const char* subversion) { - Utils().createDirectories(path); + PlatformUtils().createDirectories(path); EXPECT_TRUE(folly::writeFile( std::string(version), fmt::format("{}/{}_ver", path, deviceType).c_str())); diff --git a/fboss/platform/platform_manager/tests/PresenceCheckerTest.cpp b/fboss/platform/platform_manager/tests/PresenceCheckerTest.cpp index aa9ea3137c103..5b0f927b0cb3c 100644 --- a/fboss/platform/platform_manager/tests/PresenceCheckerTest.cpp +++ b/fboss/platform/platform_manager/tests/PresenceCheckerTest.cpp @@ -3,10 +3,12 @@ #include #include +#include "fboss/platform/helpers/MockPlatformUtils.h" #include "fboss/platform/platform_manager/PresenceChecker.h" #include "fboss/platform/platform_manager/Utils.h" using namespace ::testing; +using namespace facebook::fboss::platform; using namespace facebook::fboss::platform::platform_manager; class MockDevicePathResolver : public DevicePathResolver { @@ -33,11 +35,6 @@ class MockDevicePathResolver : public DevicePathResolver { class MockUtils : public Utils { public: MOCK_METHOD(int, getGpioLineValue, (const std::string&, int), (const)); - MOCK_METHOD( - std::optional, - getStringFileContent, - (const std::string&), - (const)); }; class PresenceCheckerTest : public testing::Test { @@ -56,7 +53,8 @@ class PresenceCheckerTest : public testing::Test { TEST_F(PresenceCheckerTest, gpioPresent) { auto utils = std::make_shared(); - PresenceChecker presenceChecker(devicePathResolver_, utils); + auto platformUtils = std::make_shared(); + PresenceChecker presenceChecker(devicePathResolver_, utils, platformUtils); PresenceDetection presenceDetection = PresenceDetection(); presenceDetection.gpioLineHandle() = GpioLineHandle(); @@ -84,7 +82,8 @@ TEST_F(PresenceCheckerTest, gpioPresent) { TEST_F(PresenceCheckerTest, sysfsPresent) { auto utils = std::make_shared(); - PresenceChecker presenceChecker(devicePathResolver_, utils); + auto platformUtils = std::make_shared(); + PresenceChecker presenceChecker(devicePathResolver_, utils, platformUtils); PresenceDetection presenceDetection = PresenceDetection(); presenceDetection.sysfsFileHandle() = SysfsFileHandle(); @@ -96,11 +95,11 @@ TEST_F(PresenceCheckerTest, sysfsPresent) { devicePathResolver_, resolvePresencePath("device/path", "presenceName")) .WillRepeatedly(Return("/file/path")); - EXPECT_CALL(*utils, getStringFileContent("/file/path")) + EXPECT_CALL(*platformUtils, getStringFileContent("/file/path")) .WillOnce(Return(std::make_optional("1"))); EXPECT_TRUE(presenceChecker.isPresent(presenceDetection, "/slot/path")); - EXPECT_CALL(*utils, getStringFileContent("/file/path")) + EXPECT_CALL(*platformUtils, getStringFileContent("/file/path")) .WillOnce(Return(std::make_optional("0"))); EXPECT_FALSE(presenceChecker.isPresent(presenceDetection, "/slot/path")); diff --git a/fboss/platform/platform_manager/tests/UtilsTest.cpp b/fboss/platform/platform_manager/tests/UtilsTest.cpp index d88b78626276e..9ef9358d0d56c 100644 --- a/fboss/platform/platform_manager/tests/UtilsTest.cpp +++ b/fboss/platform/platform_manager/tests/UtilsTest.cpp @@ -1,9 +1,7 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. -#include #include -#include #include #include "fboss/platform/platform_manager/Utils.h" @@ -20,20 +18,6 @@ std::pair makeDevicePathPair( } }; // namespace -TEST(UtilsTest, CreateParentDirectories) { - auto tmpPath = folly::test::TemporaryDirectory().path(); - EXPECT_TRUE(Utils().createDirectories(tmpPath.string())); - EXPECT_TRUE(fs::exists(tmpPath.string())); - tmpPath = tmpPath / "a"; - EXPECT_TRUE(Utils().createDirectories(tmpPath.string())); - EXPECT_TRUE(fs::exists(tmpPath.string())); - tmpPath = tmpPath / "b" / "c"; - EXPECT_TRUE(Utils().createDirectories(tmpPath.string())); - EXPECT_TRUE(fs::exists(tmpPath.string())); - EXPECT_TRUE(Utils().createDirectories("/")); - EXPECT_FALSE(Utils().createDirectories("")); -} - TEST(UtilsTest, ParseDevicePath) { EXPECT_EQ( makeDevicePathPair("/", "IDPROM"), Utils().parseDevicePath("/[IDPROM]"));