Address review comments.

project-chip · Aug 29, 2023 · 4159888 · 4159888
1 parent 7eddf88
commit 4159888
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 61 deletions.
diff --git 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
@@ -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<MTRDeviceControllerStorageDelegate> _delegate;
-    dispatch_queue_t _delegateQueue;
+    id<MTRDeviceControllerStorageDelegate> _storageDelegate;
+    dispatch_queue_t _storageDelegateQueue;
     MTRDeviceController * _controller;
     // Array of nodes with resumption info, oldest-stored first.
     NSMutableArray<NSNumber *> * _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,24 +142,24 @@ - (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;
 }
 
 - (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
@@ -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<MTRStorage> storage;
 

diff --git 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.
 //