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
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
index baf3cae..c67c48c 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -136,6 +136,7 @@
mState = State::Initialized;
mRemoveFromFabricTableOnShutdown = params.removeFromFabricTableOnShutdown;
+ mDeleteFromFabricTableOnShutdown = params.deleteFromFabricTableOnShutdown;
if (GetFabricIndex() != kUndefinedFabricIndex)
{
@@ -382,8 +383,9 @@
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 @@
// 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 979a5f0..3d6e287 100644
--- a/src/controller/CHIPDeviceController.h
+++ b/src/controller/CHIPDeviceController.h
@@ -126,16 +126,28 @@
/**
* 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
* chain are provided, then attempt to locate a fabric corresponding to the given FabricIndex.
@@ -394,6 +406,7 @@
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 115d865..84a00d4 100644
--- a/src/controller/CHIPDeviceControllerFactory.cpp
+++ b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -305,6 +305,7 @@
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 3bb560a..2602731 100644
--- a/src/controller/CHIPDeviceControllerFactory.h
+++ b/src/controller/CHIPDeviceControllerFactory.h
@@ -93,16 +93,28 @@
/**
* 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
* chain are provided, then attempt to locate a fabric corresponding to the given FabricIndex.
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
index b47fb4b..22d52cd 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceController.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -509,11 +509,15 @@
}
commissionerParams.controllerVendorId = static_cast<chip::VendorId>([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 ccdb1ca..5941a79 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
@@ -1082,22 +1082,6 @@
}
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 @@
}
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 56fe7bc..65e8eac 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;
}
