diff --git a/CMakeLists.txt b/CMakeLists.txt index fa9212546db44..a79786907d1a7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1047,6 +1047,7 @@ add_dependencies(fboss_platform_services platform_manager_handler_test weutil_crc16_ccitt_test weutil_fboss_eeprom_interface_test + weutil_parser_utils_test sensor_service_sw_test sensor_service_utils_test rackmon_test diff --git a/cmake/PlatformWeutilTest.cmake b/cmake/PlatformWeutilTest.cmake index 8cd0afea9320d..07838b4e1bee8 100644 --- a/cmake/PlatformWeutilTest.cmake +++ b/cmake/PlatformWeutilTest.cmake @@ -28,3 +28,16 @@ target_link_libraries(weutil_fboss_eeprom_interface_test ) gtest_discover_tests(weutil_fboss_eeprom_interface_test) + +add_executable(weutil_parser_utils_test + fboss/platform/weutil/test/ParserUtilsTest.cpp +) + +target_link_libraries(weutil_parser_utils_test + weutil_fboss_eeprom_interface + Folly::folly + ${GTEST} + ${LIBGMOCK_LIBRARIES} +) + +gtest_discover_tests(weutil_parser_utils_test) diff --git a/fboss/platform/platform_manager/PlatformExplorer.cpp b/fboss/platform/platform_manager/PlatformExplorer.cpp index b3a893383092f..4bb51176964c9 100644 --- a/fboss/platform/platform_manager/PlatformExplorer.cpp +++ b/fboss/platform/platform_manager/PlatformExplorer.cpp @@ -19,6 +19,7 @@ #include "fboss/platform/platform_manager/Utils.h" #include "fboss/platform/weutil/FbossEepromInterface.h" #include "fboss/platform/weutil/IoctlSmbusEepromReader.h" +#include "fboss/platform/weutil/ParserUtils.h" namespace facebook::fboss::platform::platform_manager { namespace { @@ -335,6 +336,7 @@ std::optional PlatformExplorer::getPmUnitNameFromSlot( directly with ioctl and written to the /run/devmap file. See: https://github.com/facebookexternal/fboss.bsp.arista/pull/31/files */ + uint16_t eepromOffset = *idpromConfig.offset(); if ((platformConfig_.platformName().value() == "MERU800BFA" || platformConfig_.platformName().value() == "MERU800BIA" || platformConfig_.platformName().value() == "ICECUBE800BANW" || @@ -348,9 +350,12 @@ std::optional PlatformExplorer::getPmUnitNameFromSlot( IoctlSmbusEepromReader::readEeprom( eepromDir, eepromName, - 0, + *idpromConfig.offset(), std::stoi(*idpromConfig.address(), nullptr, 16), - dataStore_.getI2cBusNum(slotPath, *idpromConfig.busName())); + dataStore_.getI2cBusNum(slotPath, *idpromConfig.busName()), + kMaxEepromDataRegionSize); + // The parsed file contains data starting at offset 0 + eepromOffset = 0; } catch (const std::exception& e) { auto errMsg = fmt::format( "Could not read MERU_SCM_EEPROM for {}: {}", @@ -373,8 +378,7 @@ std::optional PlatformExplorer::getPmUnitNameFromSlot( try { auto idpromDevicePath = Utils().createDevicePath(slotPath, "IDPROM"); dataStore_.updateEepromContents( - idpromDevicePath, - FbossEepromInterface(eepromPath, *idpromConfig.offset())); + idpromDevicePath, FbossEepromInterface(eepromPath, eepromOffset)); const auto& eepromContents = dataStore_.getEepromContents(idpromDevicePath); if (idpromDevicePath == *platformConfig_.chassisEepromDevicePath()) { diff --git a/fboss/platform/weutil/ConfigUtils.cpp b/fboss/platform/weutil/ConfigUtils.cpp index 1c6169baf6733..d736b58b80c73 100644 --- a/fboss/platform/weutil/ConfigUtils.cpp +++ b/fboss/platform/weutil/ConfigUtils.cpp @@ -160,7 +160,7 @@ std::unordered_map ConfigUtils::getFruEepromList() { std::string eepromName = "SCM"; FruEeprom fruEeprom; fruEeprom.path = "/run/devmap/eeproms/MERU_SCM_EEPROM"; - fruEeprom.offset = getEepromOffset(config_, eepromName); + fruEeprom.offset = 0; fruEepromList[eepromName] = fruEeprom; } else if (config_.platformName().value() == "DARWIN") { // Darwin Platform special case that doesn't have a chassis EEPROM path diff --git a/fboss/platform/weutil/ParserUtils.cpp b/fboss/platform/weutil/ParserUtils.cpp index fdd10d5bd3c10..71a95e91425cf 100644 --- a/fboss/platform/weutil/ParserUtils.cpp +++ b/fboss/platform/weutil/ParserUtils.cpp @@ -1,7 +1,6 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. #include #include -#include #include #include @@ -53,42 +52,27 @@ std::vector ParserUtils::loadEeprom( const std::string& eeprom, int offset) { // Declare buffer, and fill it up with 0s - long fileSize = 0; - long bytesToRead = 0; std::ifstream file(eeprom, std::ios::binary); - std::vector result; - // First, detect EEPROM size - try { - file.seekg(0, std::ios::end); - fileSize = file.tellg(); - if (fileSize < 0) { - throw std::runtime_error( - fmt::format("EEPROM {} does not exist, or is empty!", eeprom)); - } + std::vector result(kMaxEepromDataRegionSize); - // bytesToRead cannot be bigger than the remaining bytes of the file from - // the offset. That is, we cannot read beyond the end of the file. - // If the remaining bytes are smaller than max, then we only read up to - // the end of the file. - bytesToRead = fileSize - offset; - if (bytesToRead < 0) { - throw std::runtime_error("Offset greater than file size"); - } - result.resize(bytesToRead); - } catch (std::exception& ex) { - std::cout << "Failed to detect EEPROM size (" << eeprom - << "): " << ex.what() << std::endl; - throw std::runtime_error("Unable to detect EEPROM size."); + // First, validate file can be opened + if (!file) { + throw std::runtime_error(fmt::format("Failed to open EEPROM {}.", eeprom)); } // Now, read the eeprom try { file.seekg(offset, std::ios::beg); - file.read((char*)result.data(), bytesToRead); + file.read((char*)result.data(), kMaxEepromDataRegionSize); + if (file.gcount() == 0) { + throw std::runtime_error("Offset greater than file size"); + } file.close(); } catch (std::exception& ex) { - std::cout << "Failed to read EEPROM contents " << ex.what() << std::endl; + throw std::runtime_error( + fmt::format("Failed to read EEPROM contents: {}", ex.what())); } + return result; } diff --git a/fboss/platform/weutil/ParserUtils.h b/fboss/platform/weutil/ParserUtils.h index 97e43b1e3245e..da0a48ae91e12 100644 --- a/fboss/platform/weutil/ParserUtils.h +++ b/fboss/platform/weutil/ParserUtils.h @@ -7,6 +7,9 @@ namespace facebook::fboss::platform { +// Maximum size of the EEPROM data region to read. +constexpr long kMaxEepromDataRegionSize = 512; + class ParserUtils { public: static std::vector loadEeprom(const std::string& eeprom, int offset); diff --git a/fboss/platform/weutil/test/BUCK b/fboss/platform/weutil/test/BUCK index 85d7414d4df71..77d3210b353cc 100644 --- a/fboss/platform/weutil/test/BUCK +++ b/fboss/platform/weutil/test/BUCK @@ -56,3 +56,15 @@ cpp_unittest( "//thrift/lib/cpp2/protocol:protocol", ], ) + +cpp_unittest( + name = "parser_utils_test", + srcs = [ + "ParserUtilsTest.cpp", + ], + deps = [ + "//fboss/platform/weutil:fboss_eeprom_lib", + "//folly:file_util", + "//folly/testing:test_util", + ], +) diff --git a/fboss/platform/weutil/test/ParserUtilsTest.cpp b/fboss/platform/weutil/test/ParserUtilsTest.cpp new file mode 100644 index 0000000000000..065f94b0d9dbb --- /dev/null +++ b/fboss/platform/weutil/test/ParserUtilsTest.cpp @@ -0,0 +1,33 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include + +#include +#include + +#include "fboss/platform/weutil/ParserUtils.h" + +namespace facebook::fboss::platform { + +class ParserUtilsTest : public ::testing::Test { + protected: + std::string writeTempFile(const std::vector& data) { + std::string path = tmpDir_.path().string() + "/eeprom"; + folly::writeFile(data, path.c_str()); + return path; + } + + private: + folly::test::TemporaryDirectory tmpDir_; +}; + +// Verify loadEeprom() reads at most 512 bytes regardless of file size. +TEST_F(ParserUtilsTest, LargerThan512BytesReturns512Bytes) { + std::vector data(600, 0xAB); + auto path = writeTempFile(data); + auto result = ParserUtils::loadEeprom(path, 0); + EXPECT_EQ(result.size(), kMaxEepromDataRegionSize); + EXPECT_EQ(result, std::vector(kMaxEepromDataRegionSize, 0xAB)); +} + +} // namespace facebook::fboss::platform