nvmed: do not register FeatureStore on identify failure When a drive is absent or fails to respond to Identify Controller during FeatureStore initialization, do not attempt to discover features or register the FeatureStore D-Bus interface. Previously, if Identify Controller failed, oncs remained 0, which caused oncsBit4 to evaluate to false. In discoverFeatures, the !oncsBit4 fallback path unconditionally populated all default features into featureCollection and exposed xyz.openbmc_project.NVMe.FeatureStore on D-Bus. Redfish clients querying feature endpoints on absent slots then experienced D-Bus call failures mapped to HTTP 500 Internal Error. With this change, fetchIdentifyData returns false on error, skipping discoverFeatures and D-Bus interface registration, and resets the featureStore pointer in controller, subsystem, and volume classes. Tested: Ran unit tests. Google-Bug-Id: 555312695 Change-Id: I6e4e93184269fc95a5faccb42d5da5f73ed9abff Signed-off-by: Guangzong Chen <guangzong@google.com> TAG=agy CONV=b4ce846a-32f5-46f9-8f8e-b66c6d95ea2b
diff --git a/src/NVMeController.cpp b/src/NVMeController.cpp index 3b099ec..d7fe044 100644 --- a/src/NVMeController.cpp +++ b/src/NVMeController.cpp
@@ -594,11 +594,17 @@ std::make_shared<FeatureStore>(io, objServer, conn, path, nvmeIntf, nvmeCtrl, 0, FeatureScope::Controller); - this->featureStore->init([path = this->path](const std::error_code& ec) { + this->featureStore->init([selfWeak{weak_from_this()}, + path = this->path](const std::error_code& ec) { if (ec) { lg2::error("[{PATH}]Failed to initialize feature store: {ERROR}", "PATH", path, "ERROR", ec.message()); + auto self = selfWeak.lock(); + if (self) + { + self->featureStore.reset(); + } } }); }
diff --git a/src/NVMeFeatureStore.cpp b/src/NVMeFeatureStore.cpp index 672bdd3..1980ada 100644 --- a/src/NVMeFeatureStore.cpp +++ b/src/NVMeFeatureStore.cpp
@@ -222,13 +222,18 @@ (void)boost::asio::spawn( io, [self{shared_from_this()}, cb{std::move(cb)}]( const boost::asio::yield_context& yield) mutable { - self->fetchIdentifyData(yield); + if (!self->fetchIdentifyData(yield)) + { + cb(std::make_error_code(std::errc::no_such_device)); + return; + } + self->discoverFeatures(yield); self->registerDbusInterface(); cb({}); }); } -void FeatureStore::fetchIdentifyData(const boost::asio::yield_context& yield) +bool FeatureStore::fetchIdentifyData(const boost::asio::yield_context& yield) { std::string path = this->objectPath; auto [ex, data] = asyncAdminIdentify( @@ -239,16 +244,14 @@ lg2::error( "[{PATH}]Identify Controller failed during FeatureStore init: {ERROR}", "PATH", path, "ERROR", ex->what()); - discoverFeatures(yield); - return; + return false; } if (data.size() < sizeof(nvme_id_ctrl)) { lg2::error("[{PATH}]Identify Controller returned insufficient data", "PATH", path); - discoverFeatures(yield); - return; + return false; } nvme_id_ctrl idCtrl = {}; std::memcpy(&idCtrl, data.data(), sizeof(nvme_id_ctrl)); @@ -262,7 +265,7 @@ "PATH", path); } - discoverFeatures(yield); + return true; } void FeatureStore::discoverFeatures(const boost::asio::yield_context& yield)
diff --git a/src/NVMeFeatureStore.hpp b/src/NVMeFeatureStore.hpp index 3068696..90c6a3c 100644 --- a/src/NVMeFeatureStore.hpp +++ b/src/NVMeFeatureStore.hpp
@@ -63,7 +63,7 @@ // Helper to determine feature collection void discoverFeatures(const boost::asio::yield_context& yield); - void fetchIdentifyData(const boost::asio::yield_context& yield); + bool fetchIdentifyData(const boost::asio::yield_context& yield); void registerDbusInterface(); bool isFeatureSupported(const std::string& featureName) const;
diff --git a/src/NVMeSubsys.cpp b/src/NVMeSubsys.cpp index b0e10e9..2f021b6 100644 --- a/src/NVMeSubsys.cpp +++ b/src/NVMeSubsys.cpp
@@ -194,12 +194,18 @@ io, objServer, conn, path, ctrl->getNvmeIntf(), ctrl->getMiCtrl(), 0, FeatureScope::Subsystem); - this->featureStore->init([path = this->path](const std::error_code& ec) { + this->featureStore->init([selfWeak{weak_from_this()}, + path = this->path](const std::error_code& ec) { if (ec) { lg2::error( "[{PATH}]Failed to initialize feature store for subsystem: {ERROR}", "PATH", path, "ERROR", ec.message()); + auto self = selfWeak.lock(); + if (self) + { + self->featureStore.reset(); + } } }); }
diff --git a/src/NVMeVolume.cpp b/src/NVMeVolume.cpp index 6e67ee2..0e078dc 100644 --- a/src/NVMeVolume.cpp +++ b/src/NVMeVolume.cpp
@@ -183,12 +183,18 @@ conn->get_io_context(), objServer, conn, path, ctrl->getNvmeIntf(), ctrl->getMiCtrl(), namespaceId(), FeatureScope::Namespace); - this->featureStore->init([path = this->path](const std::error_code& ec) { + this->featureStore->init([selfWeak{weak_from_this()}, + path = this->path](const std::error_code& ec) { if (ec) { lg2::error( "[{PATH}]Failed to initialize feature store for volume: {ERROR}", "PATH", path, "ERROR", ec.message()); + auto self = selfWeak.lock(); + if (self) + { + self->featureStore.reset(); + } } }); }
diff --git a/tests/test_nvme_feature_store.cpp b/tests/test_nvme_feature_store.cpp index 467924a..9a16224 100644 --- a/tests/test_nvme_feature_store.cpp +++ b/tests/test_nvme_feature_store.cpp
@@ -736,6 +736,48 @@ EXPECT_TRUE(threwNotAllowedSaved); } +TEST_F(FeatureStoreTest, DiscoverFeaturesIdentifyControllerFails) +{ + boost::asio::io_context io; + auto conn = std::make_shared<sdbusplus::asio::connection>(io); + sdbusplus::asio::object_server objServer(conn); + std::string objectPath = "/xyz/openbmc_project/nvme/vol1"; + + auto mockNvme = std::make_shared<NVMeMiMock>(); + nvme_mi_ctrl_t ctrl = nullptr; + + auto featureStore = std::make_shared<FeatureStore>( + io, objServer, conn, objectPath, mockNvme, ctrl, 1, + FeatureScope::Namespace); + + // Mock Identify Controller to fail (e.g. absent drive) + EXPECT_CALL(*mockNvme, + adminIdentify(::testing::_, NVME_IDENTIFY_CNS_CTRL, + ::testing::_, ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_identify_cns, uint32_t, uint16_t, + std::function<void(nvme_ex_ptr, std::span<uint8_t>)>&& cb) { + cb(makeLibNVMeError(0, 0x02, "Identify Controller failed"), {}); + }); + + bool done = false; + std::error_code resultEc; + featureStore->init([&done, &resultEc](const std::error_code& ec) { + resultEc = ec; + done = true; + }); + + while (!done) + { + io.run_one(); + } + + EXPECT_TRUE(done); + EXPECT_EQ(resultEc, std::make_error_code(std::errc::no_such_device)); + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "TemperatureThreshold_Current")); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);