Skip to content

Commit

Permalink
Add DeviceController deleteFromFabricTableOnShutdown option (project-…
Browse files Browse the repository at this point in the history
…chip#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
  • Loading branch information
ksperling-apple authored Apr 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 9080cd3 commit 67e8110
Showing 7 changed files with 62 additions and 59 deletions.
16 changes: 9 additions & 7 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
@@ -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);
}
}

25 changes: 19 additions & 6 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
@@ -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;

1 change: 1 addition & 0 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
@@ -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;
24 changes: 18 additions & 6 deletions src/controller/CHIPDeviceControllerFactory.h
Original file line number Diff line number Diff line change
@@ -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
12 changes: 8 additions & 4 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
@@ -509,11 +509,15 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
}
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
28 changes: 0 additions & 28 deletions src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
Original file line number Diff line number Diff line change
@@ -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);
}
});
}

15 changes: 7 additions & 8 deletions src/darwin/Framework/CHIP/MTRSessionResumptionStorageBridge.mm
Original file line number Diff line number Diff line change
@@ -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;
}

0 comments on commit 67e8110

Please sign in to comment.