diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index dd776940654462..2ae83b320839db 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -143,6 +143,8 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) mSystemState = params.systemState->Retain(); mState = State::Initialized; + mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; + if (GetFabricIndex() != kUndefinedFabricIndex) { ChipLogProgress(Controller, @@ -348,10 +350,13 @@ void DeviceController::Shutdown() // existing sessions too? mSystemState->SessionMgr()->ExpireAllSessionsForFabric(mFabricIndex); - FabricTable * fabricTable = mSystemState->Fabrics(); - if (fabricTable != nullptr) + if (mRemoveFromFabricTableOnShutdown) { - fabricTable->Forget(mFabricIndex); + FabricTable * fabricTable = mSystemState->Fabrics(); + if (fabricTable != nullptr) + { + fabricTable->Forget(mFabricIndex); + } } } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 04c24db25a0eea..c3f6cd3ff49f41 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -128,6 +128,17 @@ struct ControllerInitParams // bool enableServerInteractions = false; + /** + * Controls whether shutdown of the controller removes the corresponding + * entry from the fabric table. For now the removal is just from the + * in-memory table, not from storage, which means that after controller + * shutdown the storage and the in-memory fabric table will be out of sync. + * This is acceptable for implementations that don't actually store any of + * the fabric table information, but if someone wants a true removal at some + * point another option will need to be added here. + */ + bool removeFromFabricTableOnShutdown = true; + chip::VendorId controllerVendorId; }; @@ -330,6 +341,8 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController FabricIndex mFabricIndex = kUndefinedFabricIndex; + bool mRemoveFromFabricTableOnShutdown = true; + // TODO(cecille): Make this configuarable. static constexpr int kMaxCommissionableNodes = 10; Dnssd::DiscoveredNodeData mCommissionableNodes[kMaxCommissionableNodes]; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 7180566e36507c..4cc20bfc3f10a7 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -286,6 +286,7 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerICAC = params.controllerICAC; controllerParams.controllerRCAC = params.controllerRCAC; controllerParams.permitMultiControllerFabrics = params.permitMultiControllerFabrics; + controllerParams.removeFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; controllerParams.systemState = mSystemState; controllerParams.controllerVendorId = params.controllerVendorId; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index fd77c3779017a9..1fa0563b6bd8e1 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -90,6 +90,17 @@ struct SetupParams // bool enableServerInteractions = false; + /** + * Controls whether shutdown of the controller removes the corresponding + * entry from the fabric table. For now the removal is just from the + * in-memory table, not from storage, which means that after controller + * shutdown the storage and the in-memory fabric table will be out of sync. + * This is acceptable for implementations that don't actually store any of + * the fabric table information, but if someone wants a true removal at some + * point another option will need to be added here. + */ + bool removeFromFabricTableOnShutdown = true; + Credentials::DeviceAttestationVerifier * deviceAttestationVerifier = nullptr; CommissioningDelegate * defaultCommissioner = nullptr; }; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index a436003d7605b9..2192e9bda46e81 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -336,6 +336,11 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams } commissionerParams.controllerVendorId = static_cast([startupParams.vendorID unsignedShortValue]); commissionerParams.enableServerInteractions = startupParams.advertiseOperational; + // We don't want to remove things from the fabric table on controller + // shutdown, since our controller setup depends on being able to fetch + // fabric information for the relevant fabric indices on controller + // bring-up. + commissionerParams.removeFromFabricTableOnShutdown = false; commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier; auto & factory = chip::Controller::DeviceControllerFactory::GetInstance(); diff --git a/src/darwin/Framework/CHIPTests/MTRControllerTests.m b/src/darwin/Framework/CHIPTests/MTRControllerTests.m index 06dfb7a2beb3b7..d28beef64ccb42 100644 --- a/src/darwin/Framework/CHIPTests/MTRControllerTests.m +++ b/src/darwin/Framework/CHIPTests/MTRControllerTests.m @@ -391,8 +391,29 @@ - (void)testControllerStartControllersOnTwoFabricIds XCTAssertNotNil(controller2); XCTAssertTrue([controller2 isRunning]); + // Verify that we can't start on an existing fabric while we have a + // controller on that fabric already. XCTAssertNil([factory createControllerOnExistingFabric:params2 error:nil]); + // Now test restarting the controller on the first fabric while the + // controller on the second fabric is still running. + [controller1 shutdown]; + XCTAssertFalse([controller1 isRunning]); + + controller1 = [factory createControllerOnExistingFabric:params1 error:nil]; + XCTAssertNotNil(controller1); + XCTAssertTrue([controller1 isRunning]); + + // Now test restarting the controller on the second fabric while the + // controller on the first fabric is still running. + [controller2 shutdown]; + XCTAssertFalse([controller2 isRunning]); + + controller2 = [factory createControllerOnExistingFabric:params2 error:nil]; + XCTAssertNotNil(controller2); + XCTAssertTrue([controller2 isRunning]); + + // Shut down everything. [controller1 shutdown]; XCTAssertFalse([controller1 isRunning]); diff --git a/src/darwin/Framework/CHIPTests/MTRTestStorage.h b/src/darwin/Framework/CHIPTests/MTRTestStorage.h index ba5e386be23b8c..8e0a5d6859180a 100644 --- a/src/darwin/Framework/CHIPTests/MTRTestStorage.h +++ b/src/darwin/Framework/CHIPTests/MTRTestStorage.h @@ -23,6 +23,7 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSData *)storageDataForKey:(NSString *)key; - (BOOL)setStorageData:(NSData *)value forKey:(NSString *)key; - (BOOL)removeStorageDataForKey:(NSString *)key; +- (NSString *)dumpStorageToString; @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIPTests/MTRTestStorage.m b/src/darwin/Framework/CHIPTests/MTRTestStorage.m index b03b3509891d39..8d80bd59d8b381 100644 --- a/src/darwin/Framework/CHIPTests/MTRTestStorage.m +++ b/src/darwin/Framework/CHIPTests/MTRTestStorage.m @@ -52,4 +52,9 @@ - (instancetype)init return self; } +- (NSString *)dumpStorageToString +{ + return [NSString stringWithFormat:@"%@", _values]; +} + @end