From 67e81102edc1513ebeaf890f019d7d51126dc32f Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Sat, 6 Apr 2024 01:13:43 +1300 Subject: [PATCH] Add DeviceController deleteFromFabricTableOnShutdown option (#32846) * Add DeviceController deleteFromFabricTableOnShutdown option This is analogous to removeFromFabricTableOnShutdown but does an actual Delete(), i.e. affects storage, rather than just a Forget(). Use this to simplify controller shutdown on Darwin. * Address review comments --- src/controller/CHIPDeviceController.cpp | 16 ++++++----- src/controller/CHIPDeviceController.h | 25 +++++++++++++---- .../CHIPDeviceControllerFactory.cpp | 1 + src/controller/CHIPDeviceControllerFactory.h | 24 ++++++++++++---- .../Framework/CHIP/MTRDeviceController.mm | 12 +++++--- .../CHIP/MTRDeviceControllerFactory.mm | 28 ------------------- .../CHIP/MTRSessionResumptionStorageBridge.mm | 15 +++++----- 7 files changed, 62 insertions(+), 59 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index baf3cae1da4b50..c67c48c0cce2be 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -136,6 +136,7 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) mState = State::Initialized; mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; + mDeleteFromFabricTableOnShutdown = params.deleteFromFabricTableOnShutdown; if (GetFabricIndex() != kUndefinedFabricIndex) { @@ -382,8 +383,9 @@ void DeviceController::Shutdown() VerifyOrReturn(mState != State::NotInitialized); + // If our state is initialialized it means mSystemState is valid, + // and we can use it below before we release our reference to it. ChipLogDetail(Controller, "Shutting down the controller"); - mState = State::NotInitialized; if (mFabricIndex != kUndefinedFabricIndex) @@ -401,13 +403,13 @@ void DeviceController::Shutdown() // existing sessions too? mSystemState->SessionMgr()->ExpireAllSessionsForFabric(mFabricIndex); - if (mRemoveFromFabricTableOnShutdown) + if (mDeleteFromFabricTableOnShutdown) { - FabricTable * fabricTable = mSystemState->Fabrics(); - if (fabricTable != nullptr) - { - fabricTable->Forget(mFabricIndex); - } + mSystemState->Fabrics()->Delete(mFabricIndex); + } + else if (mRemoveFromFabricTableOnShutdown) + { + mSystemState->Fabrics()->Forget(mFabricIndex); } } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 979a5f044fde8b..3d6e287ff17f44 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -126,15 +126,27 @@ struct ControllerInitParams /** * 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. + * entry from the in-memory fabric table, but NOT from storage. + * + * Note that this means that after controller shutdown the storage and + * in-memory versions of the fabric table will be out of sync. + * For compatibility reasons this is the default behavior. + * + * @see deleteFromFabricTableOnShutdown */ bool removeFromFabricTableOnShutdown = true; + /** + * Controls whether shutdown of the controller deletes the corresponding + * entry from the fabric table (both in-memory and storage). + * + * If both `removeFromFabricTableOnShutdown` and this setting are true, + * this setting will take precedence. + * + * @see removeFromFabricTableOnShutdown + */ + bool deleteFromFabricTableOnShutdown = false; + /** * Specifies whether to utilize the fabric table entry for the given FabricIndex * for initialization. If provided and neither the operational key pair nor the NOC @@ -394,6 +406,7 @@ class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController FabricIndex mFabricIndex = kUndefinedFabricIndex; bool mRemoveFromFabricTableOnShutdown = true; + bool mDeleteFromFabricTableOnShutdown = false; FabricTable::AdvertiseIdentity mAdvertiseIdentity = FabricTable::AdvertiseIdentity::Yes; diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 115d865e0ac0cb..84a00d4243acee 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -305,6 +305,7 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll controllerParams.controllerRCAC = params.controllerRCAC; controllerParams.permitMultiControllerFabrics = params.permitMultiControllerFabrics; controllerParams.removeFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown; + controllerParams.deleteFromFabricTableOnShutdown = params.deleteFromFabricTableOnShutdown; controllerParams.systemState = mSystemState; controllerParams.controllerVendorId = params.controllerVendorId; diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 3bb560a9ac321f..260273180d0ee7 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -93,15 +93,27 @@ struct SetupParams /** * 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. + * entry from the in-memory fabric table, but NOT from storage. + * + * Note that this means that after controller shutdown the storage and + * in-memory versions of the fabric table will be out of sync. + * For compatibility reasons this is the default behavior. + * + * @see deleteFromFabricTableOnShutdown */ bool removeFromFabricTableOnShutdown = true; + /** + * Controls whether shutdown of the controller deletes the corresponding + * entry from the fabric table (both in-memory and storage). + * + * If both `removeFromFabricTableOnShutdown` and this setting are true, + * this setting will take precedence. + * + * @see removeFromFabricTableOnShutdown + */ + bool deleteFromFabricTableOnShutdown = false; + /** * Specifies whether to utilize the fabric table entry for the given FabricIndex * for initialization. If provided and neither the operational key pair nor the NOC diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index b47fb4b437ec9d..22d52cd5b4a684 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -509,11 +509,15 @@ - (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. + + // We never want plain "removal" from the fabric table since this leaves + // the in-memory state out of sync with what's in storage. In per-controller + // storage mode, have the controller delete itself from the fabric table on shutdown. + // In factory storage mode we need to keep fabric information around so we can + // start another controller on that existing fabric at a later time. commissionerParams.removeFromFabricTableOnShutdown = false; + commissionerParams.deleteFromFabricTableOnShutdown = (startupParams.storageDelegate != nil); + commissionerParams.permitMultiControllerFabrics = startupParams.allowMultipleControllersPerFabric; // Set up our attestation verifier. Assume we want to use the default diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index ccdb1ca750c1e1..5941a799b5b514 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1082,22 +1082,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller } sharedCleanupBlock(); - - // Now that our per-controller storage for the controller being shut - // down is guaranteed to be disconnected, go ahead and clean up the - // fabric table entry for the controller if we're in per-controller - // storage mode. - if (self->_usingPerControllerStorage) { - // We have to use a new fabric table to do this cleanup, because - // our system state is gone now. - FabricTable fabricTable; - CHIP_ERROR err = [self _initFabricTable:fabricTable]; - if (err != CHIP_NO_ERROR) { - MTR_LOG_ERROR("Failed to clean up fabric entries. Expect things to act oddly: %" CHIP_ERROR_FORMAT, err.Format()); - } else { - fabricTable.Delete(controllerFabricIndex); - } - } } else { // Do the controller shutdown on the Matter work queue. dispatch_sync(_chipWorkQueue, ^{ @@ -1106,18 +1090,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller } sharedCleanupBlock(); - - // Now that our per-controller storage for the controller being shut - // down is guaranteed to be disconnected, go ahead and clean up the - // fabric table entry for the controller if we're in per-controller - // storage mode. - if (self->_usingPerControllerStorage) { - // Make sure to delete controllerFabricIndex from the system state's - // fabric table. We know there's a system state here, because we - // still have a running controller. - auto * systemState = _controllerFactory->GetSystemState(); - systemState->Fabrics()->Delete(controllerFabricIndex); - } }); } diff --git a/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm index 56fe7bc7beb6f8..65e8eac55ed830 100644 --- a/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm +++ b/src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm @@ -104,20 +104,19 @@ { assertChipStackLockedByCurrentThread(); - // NOTE: During controller startup, the Matter SDK thinks that the cert for - // a fabric index is changing and hence that it must remove session - // resumption data for that fabric index. For us that does not matter, - // since we don't key that data on fabric index anyway. But we do want to - // avoid doing this delete-on-startup so we can actually store session - // resumption data persistently. + // NOTE: During controller startup and shutdown, the SDK DeviceControllerFactory + // calls this method from ClearCASEResumptionStateOnFabricChange() due to fabric + // update or removal. For us that does not matter, since we don't key resumption + // data on fabric index anyway. But we do want to avoid executing the DeleteAll + // so we can actually store session resumption data persistently. // And that is the only use of DeleteAll for controllers in practice, in the // situations where we are using MTRSessionResumptionStorageBridge at all. // So just no-op this function, but verify that our assumptions hold. auto * controller = [mFactory runningControllerForFabricIndex:fabricIndex includeControllerStartingUp:NO - includeControllerShuttingDown:YES]; - VerifyOrDieWithMsg(controller == nil, Controller, "Deleting resumption storage for controller outside startup"); + includeControllerShuttingDown:NO]; + VerifyOrDieWithMsg(controller == nil, Controller, "ResumptionStorage::DeleteAll called for running controller"); return CHIP_NO_ERROR; }