From 415988803169e412c267d0668301ebffbf0a9972 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 29 Aug 2023 14:05:00 -0400 Subject: [PATCH] Address review comments. --- .../Framework/CHIP/MTRDemuxingStorage.mm | 5 +- .../CHIP/MTRDeviceControllerDataStore.mm | 110 +++++++++--------- .../CHIP/MTRDeviceControllerFactory.h | 6 +- .../CHIP/MTRDeviceControllerFactory.mm | 4 +- 4 files changed, 64 insertions(+), 61 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm index 87d0de07116056..3112ba952a4e81 100644 --- a/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm +++ b/src/darwin/Framework/CHIP/MTRDemuxingStorage.mm @@ -362,6 +362,7 @@ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) CHIP_ERROR MTRDemuxingStorage::SetIndexSpecificValue(FabricIndex index, NSString * key, NSData * data) { if ([key isEqualToString:@"n"]) { + // Index-scoped "n" is NOC. auto * controller = [mFactory runningControllerForFabricIndex:index]; if (controller == nil) { return CHIP_ERROR_PERSISTED_STORAGE_FAILED; @@ -408,6 +409,8 @@ static bool IsMemoryOnlyIndexSpecificKey(NSString * key) CHIP_ERROR MTRDemuxingStorage::DeleteInMemoryValue(NSString * key) { BOOL present = (mInMemoryStore[key] != nil); - [mInMemoryStore removeObjectForKey:key]; + if (present) { + [mInMemoryStore removeObjectForKey:key]; + } return present ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 19480d9f6dac78..f83e50daff72b4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -19,7 +19,7 @@ // TODO: FIXME: Figure out whether these are good key strings. static NSString * sResumptionNodeListKey = @"caseResumptionNodeList"; -static NSString * sLastUsedNOCKey = @"sLastUsedControllerNOC"; +static NSString * sLastUsedNOCKey = @"lastUsedControllerNOC"; static NSString * ResumptionByNodeIDKey(NSNumber * nodeID) { @@ -33,8 +33,8 @@ } @implementation MTRDeviceControllerDataStore { - id _delegate; - dispatch_queue_t _delegateQueue; + id _storageDelegate; + dispatch_queue_t _storageDelegateQueue; MTRDeviceController * _controller; // Array of nodes with resumption info, oldest-stored first. NSMutableArray * _nodesWithResumptionInfo; @@ -49,15 +49,15 @@ - (instancetype)initWithController:(MTRDeviceController *)controller } _controller = controller; - _delegate = storageDelegate; - _delegateQueue = storageDelegateQueue; + _storageDelegate = storageDelegate; + _storageDelegateQueue = storageDelegateQueue; __block id resumptionNodeList; - dispatch_sync(_delegateQueue, ^{ - resumptionNodeList = [_delegate controller:_controller - valueForKey:sResumptionNodeListKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + resumptionNodeList = [_storageDelegate controller:_controller + valueForKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); if (resumptionNodeList != nil) { if (![resumptionNodeList isKindOfClass:[NSMutableArray class]]) { @@ -84,35 +84,35 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo { auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID]; - dispatch_sync(_delegateQueue, ^{ + dispatch_sync(_storageDelegateQueue, ^{ if (oldInfo != nil) { // Remove old resumption id key. No need to do that for the // node id, because we are about to overwrite it. - [_delegate controller:_controller - removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID]; } - [_delegate controller:_controller - storeValue:resumptionInfo - forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; - [_delegate controller:_controller - storeValue:resumptionInfo - forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:resumptionInfo + forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; // Update our resumption info node list. [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; - [_delegate controller:_controller - storeValue:_nodesWithResumptionInfo - forKey:sResumptionNodeListKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + storeValue:_nodesWithResumptionInfo + forKey:sResumptionNodeListKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); } @@ -123,15 +123,15 @@ - (void)clearAllResumptionInfo for (NSNumber * nodeID in _nodesWithResumptionInfo) { auto * oldInfo = [self findResumptionInfoByNodeID:nodeID]; if (oldInfo != nil) { - dispatch_sync(_delegateQueue, ^{ - [_delegate controller:_controller - removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; - [_delegate controller:_controller - removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + [_storageDelegate controller:_controller + removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID) + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); } } @@ -142,12 +142,12 @@ - (void)clearAllResumptionInfo - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc { __block BOOL ok; - dispatch_sync(_delegateQueue, ^{ - ok = [_delegate controller:_controller - storeValue:noc - forKey:sLastUsedNOCKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + dispatch_sync(_storageDelegateQueue, ^{ + ok = [_storageDelegate controller:_controller + storeValue:noc + forKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; }); return ok ? CHIP_NO_ERROR : CHIP_ERROR_PERSISTED_STORAGE_FAILED; } @@ -155,11 +155,11 @@ - (CHIP_ERROR)storeLastUsedNOC:(MTRCertificateTLVBytes)noc - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC { __block id data; - dispatch_sync(_delegateQueue, ^{ - data = [_delegate controller:_controller - valueForKey:sLastUsedNOCKey - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityRequired]; + dispatch_sync(_storageDelegateQueue, ^{ + data = [_storageDelegate controller:_controller + valueForKey:sLastUsedNOCKey + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityRequired]; }); if (data == nil) { @@ -176,11 +176,11 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastUsedNOC - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(NSString *)key { __block id resumptionInfo; - dispatch_sync(_delegateQueue, ^{ - resumptionInfo = [_delegate controller:_controller - valueForKey:key - securityLevel:MTRStorageSecurityLevelSecure - sharingType:MTRStorageSharingTypeSameIdentityAllowed]; + dispatch_sync(_storageDelegateQueue, ^{ + resumptionInfo = [_storageDelegate controller:_controller + valueForKey:key + securityLevel:MTRStorageSecurityLevelSecure + sharingType:MTRStorageSharingTypeSameIdentityAllowed]; }); if (resumptionInfo == nil) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h index 8866b4ae55d532..6471db8b005ccb 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h @@ -40,9 +40,9 @@ API_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) /* * Storage used to store persistent information for the fabrics the * controllers ends up interacting with. This is only used if "initWithStorage" - * is used to initialize the MTRDeviceControllerFactoryParams. If init is used, - * called. If "init" is used, this property will contain a dummy storage that - * will not be used for anything. + * is used to initialize the MTRDeviceControllerFactoryParams. If "init" is + * used, this property will contain a dummy storage that will not be used for + * anything. */ @property (nonatomic, strong, readonly) id storage; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index cdd40bfb3da810..6e5f581ffe257b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -100,7 +100,7 @@ @interface MTRDeviceControllerFactory () // 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 when in a block that was +// 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 @@ -110,7 +110,7 @@ @interface MTRDeviceControllerFactory () // 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 +// 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. //