From 0c1707234b30972d8c1ce87bb541af24ff4156fa Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Tue, 16 Apr 2024 14:41:25 +1200 Subject: [PATCH] Darwin: MTRDeviceControllerFactory cleanup (#32960) * Darwin: MTRDeviceControllerFactory cleanup Internal methods don't need a named category Use ivars directly (most internal properties were never used as properties) Use MTR_DIRECT_MEMBERS for internals / implementation No need to pre-declare internal methods used only with the same file * Darwin: MTRDeviceControllerFactory cleanup (cont) Reorder ivars a little to group related things * Darwin: MTRDeviceControllerFactory cleanup (cont) Allocate C++ objects that are created at init time directly as ivars instead of as pointers. This avoids the need for manual cleanup code. * Fix darwin-framework-tool link issue * restyle --- .../Framework/CHIP/MTRDeviceController.mm | 2 +- .../CHIP/MTRDeviceControllerFactory.mm | 263 ++++++------------ .../MTRDeviceControllerFactory_Internal.h | 35 ++- 3 files changed, 113 insertions(+), 187 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index ab2bba520b2675..87490eecf6e7a6 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -551,7 +551,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams } errorCode = chip::Credentials::SetSingleIpkEpochKey( - _factory.groupData, fabricIdx, _operationalCredentialsDelegate->GetIPK(), compressedId); + _factory.groupDataProvider, fabricIdx, _operationalCredentialsDelegate->GetIPK(), compressedId); if ([self checkForStartError:errorCode logMsg:kErrorIPKInit]) { return; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 016f1c1d630d72..4b5ccc7d9477d4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -69,13 +69,9 @@ static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate"; static NSString * const kErrorSessionResumptionStorageInit = @"Init failure while creating a session resumption storage delegate"; -static NSString * const kErrorGroupProviderInit = @"Init failure while initializing group data provider"; -static NSString * const kErrorControllersInit = @"Init controllers array failure"; -static NSString * const kErrorCertificateValidityPolicyInit = @"Init certificate validity policy failure"; static NSString * const kErrorControllerFactoryInit = @"Init failure while initializing controller factory"; static NSString * const kErrorKeystoreInit = @"Init failure while initializing persistent storage keystore"; static NSString * const kErrorCertStoreInit = @"Init failure while initializing persistent storage operational certificate store"; -static NSString * const kErrorSessionKeystoreInit = @"Init failure while initializing session keystore"; static bool sExitHandlerRegistered = false; static void ShutdownOnExit() @@ -84,76 +80,6 @@ static void ShutdownOnExit() [[MTRDeviceControllerFactory sharedInstance] stopControllerFactory]; } -@interface MTRDeviceControllerFactory () { - MTRServerEndpoint * _otaProviderEndpoint; - std::unique_ptr _otaProviderDelegateBridge; -} - -@property (atomic, readonly) dispatch_queue_t chipWorkQueue; -@property (readonly) DeviceControllerFactory * controllerFactory; -@property (readonly) PersistentStorageDelegate * persistentStorageDelegate; -@property (readonly) Crypto::RawKeySessionKeystore * sessionKeystore; -// We use TestPersistentStorageDelegate just to get an in-memory store to back -// our group data provider impl. We initialize this store correctly on every -// controller startup, so don't need to actually persist it. -@property (readonly) TestPersistentStorageDelegate * groupStorageDelegate; -@property (readonly) Credentials::GroupDataProviderImpl * groupDataProvider; -@property (readonly) NSMutableArray * controllers; -@property (readonly) PersistentStorageOperationalKeystore * keystore; -@property (readonly) Credentials::PersistentStorageOpCertStore * opCertStore; -@property (readonly) MTROperationalBrowser * operationalBrowser; - -// productAttestationAuthorityCertificates and certificationDeclarationCertificates are just copied -// from MTRDeviceControllerFactoryParams. -@property (readonly, nullable) NSArray * productAttestationAuthorityCertificates; -@property (readonly, nullable) NSArray * certificationDeclarationCertificates; - -@property (readonly) BOOL advertiseOperational; -@property (nonatomic, readonly) Credentials::IgnoreCertificateValidityPeriodPolicy * certificateValidityPolicy; -@property (readonly) MTRSessionResumptionStorageBridge * sessionResumptionStorage; -// Lock used to serialize access to the "controllers" array and the -// "_controllerBeingStarted" and "_controllerBeingShutDown" ivars, since those -// need to be touched from both whatever queue is starting controllers and from -// the Matter queue. The way this lock is used assumes that: -// -// 1) The only mutating accesses to the controllers array and the ivars happen -// when the current queue is not the Matter queue or in a block that was -// sync-dispatched to the Matter queue. This is a good assumption, because -// the implementations of the functions that mutate these do sync dispatch to -// the Matter queue, which would deadlock if they were called when that queue -// was the current queue. -// -// 2) It's our API consumer's responsibility to serialize access to us from -// outside. -// -// These assumptions mean that if we are in a block that was sync-dispatched to -// the Matter queue, that block cannot race with either the Matter queue nor the -// non-Matter queue. Similarly, if we are in a situation where the Matter queue -// has been shut down, any accesses to the variables cannot race anything else. -// -// This means that: -// -// A. In a sync-dispatched block, or if the Matter queue has been shut down, we -// do not need to lock and can do read or write access. -// B. Apart from item A, mutations of the array and ivars must happen outside the -// Matter queue and must lock. -// C. Apart from item A, accesses on the Matter queue must be reads only and -// must lock. -// D. Locking around reads not from the Matter queue is OK but not required. -@property (nonatomic, readonly) os_unfair_lock controllersLock; - -@property (nonatomic, readonly, nullable) id otaProviderDelegate; -@property (nonatomic, readonly, nullable) dispatch_queue_t otaProviderDelegateQueue; - -@property (nonatomic, readonly) MTRDiagnosticLogsDownloader * diagnosticLogsDownloader; - -- (BOOL)findMatchingFabric:(FabricTable &)fabricTable - params:(MTRDeviceControllerStartupParams *)params - fabric:(const FabricInfo * _Nullable * _Nonnull)fabric; - -- (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceController * _Nonnull)controller; -@end - @interface MTRDeviceControllerFactoryParams () // Flag to keep track of whether our .storage is real consumer-provided storage @@ -162,18 +88,68 @@ @interface MTRDeviceControllerFactoryParams () @end +MTR_DIRECT_MEMBERS @implementation MTRDeviceControllerFactory { + dispatch_queue_t _chipWorkQueue; + DeviceControllerFactory * _controllerFactory; + + Credentials::IgnoreCertificateValidityPeriodPolicy _certificateValidityPolicy; + Crypto::RawKeySessionKeystore _sessionKeystore; + // We use TestPersistentStorageDelegate just to get an in-memory store to back + // our group data provider impl. We initialize this store correctly on every + // controller startup, so don't need to actually persist it. + TestPersistentStorageDelegate _groupStorageDelegate; + Credentials::GroupDataProviderImpl _groupDataProvider; + // _usingPerControllerStorage is only written once, during controller // factory start. After that it is only read, and can be read from // arbitrary threads. BOOL _usingPerControllerStorage; - - // See documentation for controllersLock above for the rules for accessing - // _controllerBeingStarted. + PersistentStorageDelegate * _persistentStorageDelegate; + MTRSessionResumptionStorageBridge * _sessionResumptionStorage; + PersistentStorageOperationalKeystore * _keystore; + Credentials::PersistentStorageOpCertStore * _opCertStore; + MTROperationalBrowser * _operationalBrowser; + + // productAttestationAuthorityCertificates and certificationDeclarationCertificates are just copied + // from MTRDeviceControllerFactoryParams. + NSArray * _Nullable _productAttestationAuthorityCertificates; + NSArray * _Nullable _certificationDeclarationCertificates; + + BOOL _advertiseOperational; + + // Lock used to serialize access to the "controllers" array and the + // "_controllerBeingStarted" and "_controllerBeingShutDown" ivars, since those + // need to be touched from both whatever queue is starting controllers and from + // the Matter queue. The way this lock is used assumes that: + // + // 1) The only mutating accesses to the controllers array and the ivars happen + // when the current queue is not the Matter queue or in a block that was + // sync-dispatched to the Matter queue. This is a good assumption, because + // the implementations of the functions that mutate these do sync dispatch to + // the Matter queue, which would deadlock if they were called when that queue + // was the current queue. + // + // 2) It's our API consumer's responsibility to serialize access to us from + // outside. + // + // These assumptions mean that if we are in a block that was sync-dispatched to + // the Matter queue, that block cannot race with either the Matter queue nor the + // non-Matter queue. Similarly, if we are in a situation where the Matter queue + // has been shut down, any accesses to the variables cannot race anything else. + // + // This means that: + // + // A. In a sync-dispatched block, or if the Matter queue has been shut down, we + // do not need to lock and can do read or write access. + // B. Apart from item A, mutations of the array and ivars must happen outside the + // Matter queue and must lock. + // C. Apart from item A, accesses on the Matter queue must be reads only and + // must lock. + // D. Locking around reads not from the Matter queue is OK but not required. + os_unfair_lock _controllersLock; + NSMutableArray * _controllers; MTRDeviceController * _controllerBeingStarted; - - // See documentation for controllersLock above for the rules for access - // _controllerBeingShutDown. MTRDeviceController * _controllerBeingShutDown; // Next available fabric index. Only valid when _controllerBeingStarted is @@ -182,6 +158,13 @@ @implementation MTRDeviceControllerFactory { // down. FabricIndex _nextAvailableFabricIndex; + id _Nullable _otaProviderDelegate; + dispatch_queue_t _Nullable _otaProviderDelegateQueue; + MTRServerEndpoint * _otaProviderEndpoint; + std::unique_ptr _otaProviderDelegateBridge; + + MTRDiagnosticLogsDownloader * _Nullable _diagnosticLogsDownloader; + // Array of all server endpoints across all controllers, used to ensure // in an atomic way that endpoint IDs are unique. NSMutableArray * _serverEndpoints; @@ -214,46 +197,22 @@ - (instancetype)init // cost to having an idle dispatch queue, and it simplifies our logic. DeviceLayer::PlatformMgrImpl().StartEventLoopTask(); - _running = NO; _chipWorkQueue = DeviceLayer::PlatformMgrImpl().GetWorkQueue(); _controllerFactory = &DeviceControllerFactory::GetInstance(); - _controllersLock = OS_UNFAIR_LOCK_INIT; - - _sessionKeystore = new chip::Crypto::RawKeySessionKeystore(); - if ([self checkForInitError:(_sessionKeystore != nullptr) logMsg:kErrorSessionKeystoreInit]) { - return nil; - } - - _groupStorageDelegate = new chip::TestPersistentStorageDelegate(); - if ([self checkForInitError:(_groupStorageDelegate != nullptr) logMsg:kErrorGroupProviderInit]) { - return nil; - } + // Initialize our default-constructed GroupDataProviderImpl. // For now default args are fine, since we are just using this for the IPK. - _groupDataProvider = new chip::Credentials::GroupDataProviderImpl(); - if ([self checkForInitError:(_groupDataProvider != nullptr) logMsg:kErrorGroupProviderInit]) { - return nil; - } - - _groupDataProvider->SetStorageDelegate(_groupStorageDelegate); - _groupDataProvider->SetSessionKeystore(_sessionKeystore); - CHIP_ERROR errorCode = _groupDataProvider->Init(); - if ([self checkForInitError:(CHIP_NO_ERROR == errorCode) logMsg:kErrorGroupProviderInit]) { - return nil; - } + _groupDataProvider.SetStorageDelegate(&_groupStorageDelegate); + _groupDataProvider.SetSessionKeystore(&_sessionKeystore); + CHIP_ERROR err = _groupDataProvider.Init(); + VerifyOrDieWithMsg(err == CHIP_NO_ERROR, NotSpecified, + "GroupDataProviderImpl::Init() failed: %" CHIP_ERROR_FORMAT, err.Format()); + _controllersLock = OS_UNFAIR_LOCK_INIT; _controllers = [[NSMutableArray alloc] init]; - if ([self checkForInitError:(_controllers != nil) logMsg:kErrorControllersInit]) { - return nil; - } - _certificateValidityPolicy = new Credentials::IgnoreCertificateValidityPeriodPolicy(); - if ([self checkForInitError:(_certificateValidityPolicy != nil) logMsg:kErrorCertificateValidityPolicyInit]) { - return nil; - } - - _serverEndpoints = [[NSMutableArray alloc] init]; _serverEndpointsLock = OS_UNFAIR_LOCK_INIT; + _serverEndpoints = [[NSMutableArray alloc] init]; return self; } @@ -261,7 +220,7 @@ - (instancetype)init - (void)dealloc { [self stopControllerFactory]; - [self cleanupInitObjects]; + _groupDataProvider.Finish(); } - (void)_assertCurrentQueueIsNotMatterQueue @@ -284,45 +243,6 @@ - (BOOL)checkIsRunning:(NSError * __autoreleasing *)error return NO; } -- (BOOL)checkForInitError:(BOOL)condition logMsg:(NSString *)logMsg -{ - if (condition) { - return NO; - } - - MTR_LOG_ERROR("Error: %@", logMsg); - - [self cleanupInitObjects]; - - return YES; -} - -- (void)cleanupInitObjects -{ - _controllers = nil; - - if (_groupDataProvider) { - _groupDataProvider->Finish(); - delete _groupDataProvider; - _groupDataProvider = nullptr; - } - - if (_groupStorageDelegate) { - delete _groupStorageDelegate; - _groupStorageDelegate = nullptr; - } - - if (_sessionKeystore) { - delete _sessionKeystore; - _sessionKeystore = nullptr; - } - - if (_certificateValidityPolicy) { - delete _certificateValidityPolicy; - _certificateValidityPolicy = nullptr; - } -} - - (void)cleanupStartupObjects { assertChipStackLockedByCurrentThread(); @@ -498,12 +418,12 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam } params.enableServerInteractions = startupParams.shouldStartServer; - params.groupDataProvider = _groupDataProvider; - params.sessionKeystore = _sessionKeystore; + params.groupDataProvider = &_groupDataProvider; + params.sessionKeystore = &_sessionKeystore; params.fabricIndependentStorage = _persistentStorageDelegate; params.operationalKeystore = _keystore; params.opCertStore = _opCertStore; - params.certificateValidityPolicy = _certificateValidityPolicy; + params.certificateValidityPolicy = &_certificateValidityPolicy; params.sessionResumptionStorage = _sessionResumptionStorage; errorCode = _controllerFactory->Init(params); if (errorCode != CHIP_NO_ERROR) { @@ -655,8 +575,8 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController * // Fall back to the factory-wide OTA provider delegate if one is not // provided in the startup params. if (otaProviderDelegate == nil) { - otaProviderDelegate = self.otaProviderDelegate; - otaProviderDelegateQueue = self.otaProviderDelegateQueue; + otaProviderDelegate = _otaProviderDelegate; + otaProviderDelegateQueue = _otaProviderDelegateQueue; } controller = [controller initWithFactory:self @@ -822,13 +742,13 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo [[MTRDeviceControllerStartupParamsInternal alloc] initForExistingFabric:fabricTable fabricIndex:fabric->GetFabricIndex() keystore:self->_keystore - advertiseOperational:self.advertiseOperational + advertiseOperational:self->_advertiseOperational params:startupParams]; if (params == nil) { fabricError = CHIP_ERROR_NO_MEMORY; } else { - params.productAttestationAuthorityCertificates = self.productAttestationAuthorityCertificates; - params.certificationDeclarationCertificates = self.certificationDeclarationCertificates; + params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates; + params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates; } return params; @@ -878,13 +798,13 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl auto * params = [[MTRDeviceControllerStartupParamsInternal alloc] initForNewFabric:fabricTable keystore:self->_keystore - advertiseOperational:self.advertiseOperational + advertiseOperational:self->_advertiseOperational params:startupParams]; if (params == nil) { fabricError = CHIP_ERROR_NO_MEMORY; } else { - params.productAttestationAuthorityCertificates = self.productAttestationAuthorityCertificates; - params.certificationDeclarationCertificates = self.certificationDeclarationCertificates; + params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates; + params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates; } return params; } @@ -993,7 +913,7 @@ - (void)resetOperationalAdvertising return; } - if (!self.advertiseOperational) { + if (!_advertiseOperational) { // No need to reset anything; we are not advertising the things that // would need to get reset. return; @@ -1012,9 +932,6 @@ - (void)resetOperationalAdvertising }); } } -@end - -@implementation MTRDeviceControllerFactory (InternalMethods) - (void)controllerShuttingDown:(MTRDeviceController *)controller { @@ -1041,7 +958,7 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller // Clear out out group keys for this fabric index, in case fabric // indices get reused later. If a new controller is started on the // same fabric it will be handed the IPK at that point. - self->_groupDataProvider->RemoveGroupKeys(fabricIndex); + self->_groupDataProvider.RemoveGroupKeys(fabricIndex); } // If there are no other controllers left, we can shut down some things. @@ -1259,7 +1176,7 @@ - (MTRDeviceController * _Nullable)initializeController:(MTRDeviceController *)c startupParams:parameters fabricChecker:^MTRDeviceControllerStartupParamsInternal *( FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) { - auto advertiseOperational = self.advertiseOperational && parameters.shouldAdvertiseOperational; + auto advertiseOperational = self->_advertiseOperational && parameters.shouldAdvertiseOperational; auto * params = [[MTRDeviceControllerStartupParamsInternal alloc] initForNewController:controller fabricTable:fabricTable @@ -1269,10 +1186,10 @@ - (MTRDeviceController * _Nullable)initializeController:(MTRDeviceController *)c error:fabricError]; if (params != nil) { if (params.productAttestationAuthorityCertificates == nil) { - params.productAttestationAuthorityCertificates = self.productAttestationAuthorityCertificates; + params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates; } if (params.certificationDeclarationCertificates == nil) { - params.certificationDeclarationCertificates = self.certificationDeclarationCertificates; + params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates; } } return params; @@ -1285,9 +1202,9 @@ - (PersistentStorageDelegate *)storageDelegate return _persistentStorageDelegate; } -- (Credentials::GroupDataProvider *)groupData +- (Credentials::GroupDataProvider *)groupDataProvider { - return _groupDataProvider; + return &_groupDataProvider; } @end diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index 0f157b8f89092f..c35a0b4a1692f0 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -28,6 +28,7 @@ #import #import +#import "MTRDefines_Internal.h" #import "MTRDeviceControllerFactory.h" #include @@ -44,7 +45,8 @@ namespace Credentials { NS_ASSUME_NONNULL_BEGIN -@interface MTRDeviceControllerFactory (InternalMethods) +MTR_DIRECT_MEMBERS +@interface MTRDeviceControllerFactory () - (void)controllerShuttingDown:(MTRDeviceController *)controller; @@ -104,6 +106,25 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint; +@property (readonly) chip::PersistentStorageDelegate * storageDelegate; +@property (readonly) chip::Credentials::GroupDataProvider * groupDataProvider; + +@end + +MTR_DIRECT_MEMBERS +@interface MTRDeviceControllerFactoryParams () +/* + * Initialize the device controller factory without storage. In this mode, + * device controllers will need to have per-controller storage provided to allow + * storing controller-specific information. + */ +- (instancetype)initWithoutStorage; +@end + +// Methods accessed from MTRServerAccessControl linked into darwin-framework-tool +// TODO: https://github.com/project-chip/connectedhomeip/issues/32991 +@interface MTRDeviceControllerFactory () + /** * Get the access grants that apply for the given fabric index and cluster path. * @@ -123,18 +144,6 @@ NS_ASSUME_NONNULL_BEGIN */ - (nullable NSNumber *)neededReadPrivilegeForClusterID:(NSNumber *)clusterID attributeID:(NSNumber *)attributeID; -@property (readonly) chip::PersistentStorageDelegate * storageDelegate; -@property (readonly) chip::Credentials::GroupDataProvider * groupData; - -@end - -@interface MTRDeviceControllerFactoryParams () -/* - * Initialize the device controller factory without storage. In this mode, - * device controllers will need to have per-controller storage provided to allow - * storing controller-specific information. - */ -- (instancetype)initWithoutStorage; @end NS_ASSUME_NONNULL_END