From 9491d8ff2db57e6cb84e117d56c4cc936736c09c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 16:11:59 -0400 Subject: [PATCH 1/6] Remove shutdown implementation from base MTRDeviceController. (#35676) shutdown is overriden by both MTRDeviceController_Concrete and MTRDeviceController_XPC, so the base class implementation is not reachable. And it's the only caller of finalShutdown, so that can also be removed. Also, addRunAssertion/removeRunAssertion, are only used on concrete controllers and can be removed from the base class and from MTRDeviceController_Internal. With those removed, matchesPendingShutdownControllerWithOperationalCertificate would always return false, so that can be changed accordingly, and then clearPendingShutdown becomes unreachable. At this point _keepRunningAssertionCounter and _shutdownPending are never read and can be removed. And _assertionLock is never acquired, so it can also be removed. --- .../Framework/CHIP/MTRDeviceController.mm | 85 ++----------------- .../CHIP/MTRDeviceControllerFactory.mm | 3 + .../CHIP/MTRDeviceController_Concrete.h | 15 ++++ .../CHIP/MTRDeviceController_Internal.h | 14 --- 4 files changed, 26 insertions(+), 91 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index e39a4d14e75612..4c3afe8412040b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -26,6 +26,7 @@ #import "MTRCommissionableBrowserResult_Internal.h" #import "MTRCommissioningParameters.h" #import "MTRConversion.h" +#import "MTRDefines_Internal.h" #import "MTRDeviceControllerDelegateBridge.h" #import "MTRDeviceControllerFactory_Internal.h" #import "MTRDeviceControllerLocalTestStorage.h" @@ -160,11 +161,6 @@ @implementation MTRDeviceController { // specific queue, so can't race against each other. std::atomic _suspended; - // Counters to track assertion status and access controlled by the _assertionLock - NSUInteger _keepRunningAssertionCounter; - BOOL _shutdownPending; - os_unfair_lock _assertionLock; - NSMutableArray * _delegates; id _strongDelegateForSetDelegateAPI; } @@ -183,11 +179,6 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended } _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT; - // Setup assertion variables - _keepRunningAssertionCounter = 0; - _shutdownPending = NO; - _assertionLock = OS_UNFAIR_LOCK_INIT; - _suspended = startSuspended; _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; @@ -231,11 +222,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory // before we start doing anything else with the controller. _uniqueIdentifier = uniqueIdentifier; - // Setup assertion variables - _keepRunningAssertionCounter = 0; - _shutdownPending = NO; - _assertionLock = OS_UNFAIR_LOCK_INIT; - _suspended = startSuspended; if (storageDelegate != nil) { @@ -478,75 +464,21 @@ - (void)_controllerResumed - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate { - if (!operationalCertificate || !rootCertificate) { - return FALSE; - } - NSNumber * nodeID = [MTRDeviceControllerParameters nodeIDFromNOC:operationalCertificate]; - NSNumber * fabricID = [MTRDeviceControllerParameters fabricIDFromNOC:operationalCertificate]; - NSData * publicKey = [MTRDeviceControllerParameters publicKeyFromCertificate:rootCertificate]; - - std::lock_guard lock(_assertionLock); - - // If any of the local above are nil, the return will be false since MTREqualObjects handles them correctly - return _keepRunningAssertionCounter > 0 && _shutdownPending && MTREqualObjects(nodeID, self.nodeID) && MTREqualObjects(fabricID, self.fabricID) && MTREqualObjects(publicKey, self.rootPublicKey); -} - -- (void)addRunAssertion -{ - std::lock_guard lock(_assertionLock); - - // Only take an assertion if running - if ([self isRunning]) { - ++_keepRunningAssertionCounter; - MTR_LOG("%@ Adding keep running assertion, total %lu", self, static_cast(_keepRunningAssertionCounter)); - } -} - -- (void)removeRunAssertion; -{ - std::lock_guard lock(_assertionLock); - - if (_keepRunningAssertionCounter > 0) { - --_keepRunningAssertionCounter; - MTR_LOG("%@ Removing keep running assertion, total %lu", self, static_cast(_keepRunningAssertionCounter)); - - if ([self isRunning] && _keepRunningAssertionCounter == 0 && _shutdownPending) { - MTR_LOG("%@ All assertions removed and shutdown is pending, shutting down", self); - [self finalShutdown]; - } - } + // TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its + // declaration moved to MTRDeviceController_Concrete. + return NO; } - (void)clearPendingShutdown { - std::lock_guard lock(_assertionLock); - _shutdownPending = NO; + // TODO: Once the factory knows it's dealing with MTRDeviceController_Concrete, this can be removed, and its + // declaration moved to MTRDeviceController_Concrete. + MTR_ABSTRACT_METHOD(); } - (void)shutdown { - std::lock_guard lock(_assertionLock); - - if (_keepRunningAssertionCounter > 0) { - MTR_LOG("%@ Pending shutdown since %lu assertions are present", self, static_cast(_keepRunningAssertionCounter)); - _shutdownPending = YES; - return; - } - [self finalShutdown]; -} - -- (void)finalShutdown -{ - os_unfair_lock_assert_owner(&_assertionLock); - - MTR_LOG("%@ shutdown called", self); - if (_cppCommissioner == nullptr) { - // Already shut down. - return; - } - - MTR_LOG("Shutting down %@: %@", NSStringFromClass(self.class), self); - [self cleanupAfterStartup]; + MTR_ABSTRACT_METHOD(); } // Clean up from a state where startup was called. @@ -604,7 +536,6 @@ - (void)shutDownCppController _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr); } } - _shutdownPending = NO; } - (void)deinitFromFactory diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index ee0f330b26cfc2..c44b6e3bfb25dc 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1144,6 +1144,9 @@ - (nullable MTRDeviceController *)_findPendingShutdownControllerWithOperationalC { std::lock_guard lock(_controllersLock); for (MTRDeviceController * controller in _controllers) { + // TODO: Once we know our controllers are MTRDeviceController_Concrete, move + // matchesPendingShutdownControllerWithOperationalCertificate and clearPendingShutdown to that + // interface and remove them from base MTRDeviceController_Internal. if ([controller matchesPendingShutdownControllerWithOperationalCertificate:operationalCertificate andRootCertificate:rootCertificate]) { MTR_LOG("%@ Found existing controller %@ that is pending shutdown and matching parameters, re-using it", self, controller); [controller clearPendingShutdown]; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index b491e3f140c85a..59dd42c04a97f4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -23,6 +23,21 @@ NS_ASSUME_NONNULL_BEGIN @interface MTRDeviceController_Concrete : MTRDeviceController + +/** + * Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion + * is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases + * the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release + * the assertion. + */ +- (void)addRunAssertion; + +/** + * Removes an assertion to allow the controller to shutdown once all assertions have been released. + * Invoking this method once all assertions have been released in a noop. + */ +- (void)removeRunAssertion; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 993298234f33ef..8eb21aef64eb9e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -308,20 +308,6 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion; -/** - * Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion - * is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases - * the number of assertions and needs to be matched with equal amount of '-[MTRDeviceController removeRunAssertion]` to release - * the assertion. - */ -- (void)addRunAssertion; - -/** - * Removes an assertion to allow the controller to shutdown once all assertions have been released. - * Invoking this method once all assertions have been released in a noop. - */ -- (void)removeRunAssertion; - /** * This method returns TRUE if this controller matches the fabric reference and node ID as listed in the parameters. */ From 8318df939b12549a1b1adee21d271e29cda09544 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 16:49:20 -0400 Subject: [PATCH 2/6] Clean up MTRDeviceController initWithParameters dispatch. (#35677) --- .../Framework/CHIP/MTRDeviceController.mm | 31 +++++++---- .../CHIP/MTRDeviceControllerXPCParameters.h | 6 +-- .../CHIP/MTRDeviceControllerXPCParameters.mm | 16 +++--- .../CHIP/MTRDeviceController_Concrete.mm | 52 ++++--------------- src/darwin/Framework/CHIP/MTRDevice_XPC.mm | 1 - 5 files changed, 45 insertions(+), 61 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 4c3afe8412040b..83c6faeef7a337 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -32,8 +32,10 @@ #import "MTRDeviceControllerLocalTestStorage.h" #import "MTRDeviceControllerStartupParams.h" #import "MTRDeviceControllerStartupParams_Internal.h" +#import "MTRDeviceControllerXPCParameters.h" #import "MTRDeviceController_Concrete.h" #import "MTRDeviceController_XPC.h" +#import "MTRDeviceController_XPC_Internal.h" #import "MTRDevice_Concrete.h" #import "MTRDevice_Internal.h" #import "MTRError_Internal.h" @@ -190,20 +192,31 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended - (nullable MTRDeviceController *)initWithParameters:(MTRDeviceControllerAbstractParameters *)parameters error:(NSError * __autoreleasing *)error { + // Dispatch to the right non-abstract implementation. if ([parameters isKindOfClass:MTRXPCDeviceControllerParameters.class]) { MTR_LOG("Starting up with XPC Device Controller Parameters: %@", parameters); return [[MTRDeviceController_XPC alloc] initWithParameters:parameters error:error]; - } else if (![parameters isKindOfClass:MTRDeviceControllerParameters.class]) { - MTR_LOG_ERROR("Unsupported type of MTRDeviceControllerAbstractParameters: %@", parameters); - if (error) { - *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; - } - return nil; } - auto * controllerParameters = static_cast(parameters); - // MTRDeviceControllerFactory will auto-start in per-controller-storage mode if necessary - return [MTRDeviceControllerFactory.sharedInstance initializeController:[MTRDeviceController_Concrete alloc] withParameters:controllerParameters error:error]; + if ([parameters isKindOfClass:MTRDeviceControllerMachServiceXPCParameters.class]) { + // TODO: This will need to at least pass in the uniqueIdentifier, no? initWithMachServiceName:options: seems to + // be declared but not actually implemented... + auto * xpcParameters = static_cast(parameters); + + MTR_LOG("Starting up with Mach Service XPC Device Controller Parameters: %@", parameters); + return [[MTRDeviceController_XPC alloc] initWithMachServiceName:xpcParameters.machServiceName options:xpcParameters.connectionOptions]; + } + + if ([parameters isKindOfClass:MTRDeviceControllerParameters.class]) { + MTR_LOG("Starting up with Device Controller Parameters: %@", parameters); + return [[MTRDeviceController_Concrete alloc] initWithParameters:parameters error:error]; + } + + MTR_LOG_ERROR("Unsupported type of MTRDeviceControllerAbstractParameters: %@", parameters); + if (error) { + *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; + } + return nil; } - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.h b/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.h index 9218ff2a990dcd..ac58e30cf0851a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.h @@ -18,11 +18,11 @@ NS_ASSUME_NONNULL_BEGIN -@interface MTRDeviceControllerXPCParameters : MTRDeviceControllerParameters -@end +@interface MTRDeviceControllerMachServiceXPCParameters : MTRDeviceControllerAbstractParameters -@interface MTRDeviceControllerMachServiceXPCParameters : MTRDeviceControllerXPCParameters +- (nullable instancetype)initWithUniqueIdentifier:(NSUUID *)uniqueIdentifier; +@property (atomic, retain) NSUUID * uniqueIdentifier; @property (atomic, retain) NSString * machServiceName; @property (atomic, readwrite) NSXPCConnectionOptions connectionOptions; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.mm index 5178873ec27f1e..df3c7a8ed15eab 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerXPCParameters.mm @@ -15,17 +15,19 @@ */ #import "MTRDeviceControllerXPCParameters.h" +#import "MTRDeviceControllerStartupParams_Internal.h" -@implementation MTRDeviceControllerXPCParameters +@implementation MTRDeviceControllerMachServiceXPCParameters -+ (BOOL)supportsSecureCoding +- (nullable instancetype)initWithUniqueIdentifier:(NSUUID *)uniqueIdentifier { - return YES; -} + if (!(self = [super _initInternal])) { + return nil; + } -@end - -@implementation MTRDeviceControllerMachServiceXPCParameters + _uniqueIdentifier = uniqueIdentifier; + return self; +} + (BOOL)supportsSecureCoding { diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 97f47a50faca06..8784b61d63cb88 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -31,10 +31,7 @@ #import "MTRDeviceControllerLocalTestStorage.h" #import "MTRDeviceControllerStartupParams.h" #import "MTRDeviceControllerStartupParams_Internal.h" -#import "MTRDeviceControllerXPCParameters.h" #import "MTRDeviceController_Concrete.h" -#import "MTRDeviceController_XPC.h" -#import "MTRDeviceController_XPC_Internal.h" #import "MTRDevice_Concrete.h" #import "MTRDevice_Internal.h" #import "MTRError_Internal.h" @@ -148,49 +145,22 @@ @implementation MTRDeviceController_Concrete { - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParameters *)parameters error:(NSError * __autoreleasing *)error { - /// IF YOU ARE ALARMED BY TYPES: You are right to be alarmed, but do not panic. - /// _ORDER MATTERS HERE:_ XPC parameters are a subclass of `MTRDeviceControllerParameters` - /// because of the enormous overlap of params. - if ([parameters isKindOfClass:MTRDeviceControllerXPCParameters.class]) { - if ([parameters isKindOfClass:MTRDeviceControllerMachServiceXPCParameters.class]) { - MTRDeviceControllerMachServiceXPCParameters * xpcParameters = (MTRDeviceControllerMachServiceXPCParameters *) parameters; - MTR_LOG_DEBUG("%s: got XPC parameters, getting XPC device controller", __PRETTY_FUNCTION__); - - NSString * machServiceName = xpcParameters.machServiceName; - MTR_LOG_DEBUG("%s: machServiceName %@", __PRETTY_FUNCTION__, machServiceName); - - MTRDeviceController * xpcDeviceController = [[MTRDeviceController_XPC alloc] initWithMachServiceName:machServiceName options:xpcParameters.connectionOptions]; - - /// Being of sound mind, I willfully and voluntarily make this static cast. - return static_cast(xpcDeviceController); - } else { - MTR_LOG_ERROR("%s: unrecognized XPC parameters class %@", __PRETTY_FUNCTION__, NSStringFromClass(parameters.class)); - - // TODO: there's probably a more appropriate error here. - if (error) { - *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NOT_IMPLEMENTED]; - } - - return nil; - } - } else if ([parameters isKindOfClass:MTRDeviceControllerParameters.class]) { - MTR_LOG_DEBUG("%s: got standard parameters, getting standard device controller from factory", __PRETTY_FUNCTION__); - auto * controllerParameters = static_cast(parameters); - - // Start us up normally. MTRDeviceControllerFactory will auto-start in per-controller-storage mode if necessary. - MTRDeviceControllerFactory * factory = MTRDeviceControllerFactory.sharedInstance; - id controller = [factory initializeController:self - withParameters:controllerParameters - error:error]; - return controller; - } else { - // way out of our league - MTR_LOG_ERROR("Unsupported type of MTRDeviceControllerAbstractParameters: %@", parameters); + if (![parameters isKindOfClass:MTRDeviceControllerParameters.class]) { + MTR_LOG_ERROR("Expected MTRDeviceControllerParameters but got: %@", parameters); if (error) { *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; } return nil; } + + auto * controllerParameters = static_cast(parameters); + + // Start us up normally. MTRDeviceControllerFactory will auto-start in per-controller-storage mode if necessary. + MTRDeviceControllerFactory * factory = MTRDeviceControllerFactory.sharedInstance; + id controller = [factory initializeController:self + withParameters:controllerParameters + error:error]; + return controller; } - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory diff --git a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm index a8d9a48e9cbb35..0827ee46602449 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm @@ -39,7 +39,6 @@ #import "MTRDeviceControllerLocalTestStorage.h" #import "MTRDeviceControllerStartupParams.h" #import "MTRDeviceControllerStartupParams_Internal.h" -#import "MTRDeviceControllerXPCParameters.h" #import "MTRDeviceController_Concrete.h" #import "MTRDeviceController_XPC.h" #import "MTRDevice_Concrete.h" From c845e54be4cf3b3e2a8fdea95c630311c61f1a00 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 19 Sep 2024 18:17:36 -0400 Subject: [PATCH 3/6] Additional socket checks for socket inet implementations (#35674) * Additional socket checks for socket inet implementations * Self-review update * One more extra check to make clang-tidy happy * Switch to nolint * Avoid large code deltas * Restyle * Restyled by clang-format --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io --- src/inet/TCPEndPointImplSockets.cpp | 2 ++ src/inet/UDPEndPointImplSockets.cpp | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/inet/TCPEndPointImplSockets.cpp b/src/inet/TCPEndPointImplSockets.cpp index 6b8965b19d2b4b..96cdc67d73674c 100644 --- a/src/inet/TCPEndPointImplSockets.cpp +++ b/src/inet/TCPEndPointImplSockets.cpp @@ -127,6 +127,7 @@ CHIP_ERROR TCPEndPointImplSockets::BindImpl(IPAddressType addrType, const IPAddr if (res == CHIP_NO_ERROR) { + // NOLINTNEXTLINE(clang-analyzer-unix.StdCLibraryFunctions): GetSocket calls ensure mSocket is valid if (bind(mSocket, &sa.any, sockaddrsize) != 0) { res = CHIP_ERROR_POSIX(errno); @@ -248,6 +249,7 @@ CHIP_ERROR TCPEndPointImplSockets::ConnectImpl(const IPAddress & addr, uint16_t return INET_ERROR_WRONG_ADDRESS_TYPE; } + // NOLINTNEXTLINE(clang-analyzer-unix.StdCLibraryFunctions): GetSocket calls ensure mSocket is valid int conRes = connect(mSocket, &sa.any, sockaddrsize); if (conRes == -1 && errno != EINPROGRESS) diff --git a/src/inet/UDPEndPointImplSockets.cpp b/src/inet/UDPEndPointImplSockets.cpp index c80db4d771b10f..31b16c0f8ec192 100644 --- a/src/inet/UDPEndPointImplSockets.cpp +++ b/src/inet/UDPEndPointImplSockets.cpp @@ -106,6 +106,8 @@ CHIP_ERROR IPv6Bind(int socket, const IPAddress & address, uint16_t port, Interf sa.sin6_scope_id = static_cast(interfaceId); CHIP_ERROR status = CHIP_NO_ERROR; + + // NOLINTNEXTLINE(clang-analyzer-unix.StdCLibraryFunctions): Function called only with valid socket after GetSocket if (bind(socket, reinterpret_cast(&sa), static_cast(sizeof(sa))) != 0) { status = CHIP_ERROR_POSIX(errno); @@ -139,6 +141,8 @@ CHIP_ERROR IPv4Bind(int socket, const IPAddress & address, uint16_t port) sa.sin_addr = address.ToIPv4(); CHIP_ERROR status = CHIP_NO_ERROR; + + // NOLINTNEXTLINE(clang-analyzer-unix.StdCLibraryFunctions): Function called only with valid socket after GetSocket if (bind(socket, reinterpret_cast(&sa), static_cast(sizeof(sa))) != 0) { status = CHIP_ERROR_POSIX(errno); @@ -410,6 +414,7 @@ CHIP_ERROR UDPEndPointImplSockets::SendMsgImpl(const IPPacketInfo * aPktInfo, Sy #endif // INET_CONFIG_UDP_SOCKET_PKTINFO // Send IP packet. + // NOLINTNEXTLINE(clang-analyzer-unix.StdCLibraryFunctions): GetSocket calls ensure mSocket is valid const ssize_t lenSent = sendmsg(mSocket, &msgHeader, 0); if (lenSent == -1) { From 204b1fb266f33e5cbf845ed0c59b99ae8efe0612 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 20:05:13 -0400 Subject: [PATCH 4/6] MTRDeviceControllerOverXPC should inherit from MTRDeviceController_Concrete for now. (#35685) That's what it used to do, and for now we should just leave it be. Longer-term, we need to figure out what should happen with MTRDeviceControllerOverXPC; whether it should inherit from the base MTRDeviceController, or just not exist, or what. --- src/darwin/Framework/CHIP/MTRDeviceControllerOverXPC.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerOverXPC.h b/src/darwin/Framework/CHIP/MTRDeviceControllerOverXPC.h index 9616e22b7a494c..669baea9588f57 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerOverXPC.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerOverXPC.h @@ -16,7 +16,8 @@ */ #import -#import + +#import "MTRDeviceController_Concrete.h" NS_ASSUME_NONNULL_BEGIN @@ -24,7 +25,7 @@ NS_ASSUME_NONNULL_BEGIN typedef NSXPCConnection * _Nonnull (^MTRXPCConnectBlock)(void); -@interface MTRDeviceControllerOverXPC : MTRDeviceController +@interface MTRDeviceControllerOverXPC : MTRDeviceController_Concrete - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; From dffaaf007fb905b0ac46b2e3c5b69591d972f43f Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 20:38:02 -0400 Subject: [PATCH 5/6] Move initWithFactory: and startup: declarations to MTRDeviceController_Concrete. (#35683) initWithFactory: and startup: are only called from _startDeviceController:, which was only being called with MTRDeviceController_Concrete instances. Also fixes the argument type declarations for _startDeviceController: and initializeController: to make the types compile-time enforced. The MTRDeviceController implementations of initWithFactory: and startup: are now obviously unreachable and can be removed. Some static variables that are now unused have to be removed so the compiler doesn't complain about them. Further cleanup of now-unused things will follow in separate PRs. --- .../Framework/CHIP/MTRDeviceController.mm | 410 ------------------ .../CHIP/MTRDeviceControllerFactory.mm | 6 +- .../MTRDeviceControllerFactory_Internal.h | 5 +- .../CHIP/MTRDeviceController_Concrete.h | 30 ++ .../CHIP/MTRDeviceController_Concrete.mm | 20 +- .../CHIP/MTRDeviceController_Internal.h | 30 -- 6 files changed, 46 insertions(+), 455 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 83c6faeef7a337..aaf1289f184f98 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -88,26 +88,15 @@ // TODO: These strings and their consumers in this file should probably go away, // since none of them really apply to all controllers. -static NSString * const kErrorCommissionerInit = @"Init failure while initializing a commissioner"; -static NSString * const kErrorIPKInit = @"Init failure while initializing IPK"; -static NSString * const kErrorSigningKeypairInit = @"Init failure while creating signing keypair bridge"; -static NSString * const kErrorOperationalCredentialsInit = @"Init failure while creating operational credentials delegate"; -static NSString * const kErrorOperationalKeypairInit = @"Init failure while creating operational keypair bridge"; -static NSString * const kErrorPairingInit = @"Init failure while creating a pairing delegate"; -static NSString * const kErrorPartialDacVerifierInit = @"Init failure while creating a partial DAC verifier"; static NSString * const kErrorPairDevice = @"Failure while pairing the device"; static NSString * const kErrorStopPairing = @"Failure while trying to stop the pairing process"; static NSString * const kErrorOpenPairingWindow = @"Open Pairing Window failed"; static NSString * const kErrorNotRunning = @"Controller is not running. Call startup first."; static NSString * const kErrorSetupCodeGen = @"Generating Manual Pairing Code failed"; -static NSString * const kErrorGenerateNOC = @"Generating operational certificate failed"; -static NSString * const kErrorKeyAllocation = @"Generating new operational key failed"; -static NSString * const kErrorCSRValidation = @"Extracting public key from CSR failed"; static NSString * const kErrorGetCommissionee = @"Failure obtaining device being commissioned"; static NSString * const kErrorGetAttestationChallenge = @"Failure getting attestation challenge"; static NSString * const kErrorSpake2pVerifierGenerationFailed = @"PASE verifier generation failed"; static NSString * const kErrorSpake2pVerifierSerializationFailed = @"PASE verifier serialization failed"; -static NSString * const kErrorCDCertStoreInit = @"Init failure while initializing Certificate Declaration Signing Keys store"; typedef void (^SyncWorkQueueBlock)(void); typedef id (^SyncWorkQueueBlockWithReturnValue)(void); @@ -219,150 +208,6 @@ - (nullable MTRDeviceController *)initWithParameters:(MTRDeviceControllerAbstrac return nil; } -- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory - queue:(dispatch_queue_t)queue - storageDelegate:(id _Nullable)storageDelegate - storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - otaProviderDelegate:(id _Nullable)otaProviderDelegate - otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue - uniqueIdentifier:(NSUUID *)uniqueIdentifier - concurrentSubscriptionPoolSize:(NSUInteger)concurrentSubscriptionPoolSize - storageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration - startSuspended:(BOOL)startSuspended -{ - if (self = [super init]) { - // Make sure our storage is all set up to work as early as possible, - // before we start doing anything else with the controller. - _uniqueIdentifier = uniqueIdentifier; - - _suspended = startSuspended; - - if (storageDelegate != nil) { - if (storageDelegateQueue == nil) { - MTR_LOG_ERROR("storageDelegate provided without storageDelegateQueue"); - return nil; - } - - id storageDelegateToUse = storageDelegate; - if (MTRDeviceControllerLocalTestStorage.localTestStorageEnabled) { - storageDelegateToUse = [[MTRDeviceControllerLocalTestStorage alloc] initWithPassThroughStorage:storageDelegate]; - } - _controllerDataStore = [[MTRDeviceControllerDataStore alloc] initWithController:self - storageDelegate:storageDelegateToUse - storageDelegateQueue:storageDelegateQueue]; - if (_controllerDataStore == nil) { - return nil; - } - } else { - if (MTRDeviceControllerLocalTestStorage.localTestStorageEnabled) { - dispatch_queue_t localTestStorageQueue = dispatch_queue_create("org.csa-iot.matter.framework.devicecontroller.localteststorage", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL); - MTRDeviceControllerLocalTestStorage * localTestStorage = [[MTRDeviceControllerLocalTestStorage alloc] initWithPassThroughStorage:nil]; - _controllerDataStore = [[MTRDeviceControllerDataStore alloc] initWithController:self - storageDelegate:localTestStorage - storageDelegateQueue:localTestStorageQueue]; - if (_controllerDataStore == nil) { - return nil; - } - } - } - - // Ensure the otaProviderDelegate, if any, is valid. - if (otaProviderDelegate == nil && otaProviderDelegateQueue != nil) { - MTR_LOG_ERROR("Must have otaProviderDelegate when we have otaProviderDelegateQueue"); - return nil; - } - - if (otaProviderDelegate != nil && otaProviderDelegateQueue == nil) { - MTR_LOG_ERROR("Must have otaProviderDelegateQueue when we have otaProviderDelegate"); - return nil; - } - - if (otaProviderDelegate != nil) { - if (![otaProviderDelegate respondsToSelector:@selector(handleQueryImageForNodeID:controller:params:completion:)] - && ![otaProviderDelegate respondsToSelector:@selector(handleQueryImageForNodeID:controller:params:completionHandler:)]) { - MTR_LOG_ERROR("Error: MTROTAProviderDelegate does not support handleQueryImageForNodeID"); - return nil; - } - if (![otaProviderDelegate respondsToSelector:@selector(handleApplyUpdateRequestForNodeID:controller:params:completion:)] - && ![otaProviderDelegate respondsToSelector:@selector(handleApplyUpdateRequestForNodeID:controller:params:completionHandler:)]) { - MTR_LOG_ERROR("Error: MTROTAProviderDelegate does not support handleApplyUpdateRequestForNodeID"); - return nil; - } - if (![otaProviderDelegate respondsToSelector:@selector(handleNotifyUpdateAppliedForNodeID:controller:params:completion:)] - && ![otaProviderDelegate - respondsToSelector:@selector(handleNotifyUpdateAppliedForNodeID:controller:params:completionHandler:)]) { - MTR_LOG_ERROR("Error: MTROTAProviderDelegate does not support handleNotifyUpdateAppliedForNodeID"); - return nil; - } - if (![otaProviderDelegate respondsToSelector:@selector(handleBDXTransferSessionBeginForNodeID:controller:fileDesignator:offset:completion:)] - && ![otaProviderDelegate respondsToSelector:@selector(handleBDXTransferSessionBeginForNodeID:controller:fileDesignator:offset:completionHandler:)]) { - MTR_LOG_ERROR("Error: MTROTAProviderDelegate does not support handleBDXTransferSessionBeginForNodeID"); - return nil; - } - if (![otaProviderDelegate respondsToSelector:@selector(handleBDXQueryForNodeID:controller:blockSize:blockIndex:bytesToSkip:completion:)] - && ![otaProviderDelegate respondsToSelector:@selector(handleBDXQueryForNodeID:controller:blockSize:blockIndex:bytesToSkip:completionHandler:)]) { - MTR_LOG_ERROR("Error: MTROTAProviderDelegate does not support handleBDXQueryForNodeID"); - return nil; - } - } - - _otaProviderDelegate = otaProviderDelegate; - _otaProviderDelegateQueue = otaProviderDelegateQueue; - - _chipWorkQueue = queue; - _factory = factory; - _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT; - _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; - _serverEndpoints = [[NSMutableArray alloc] init]; - _commissionableBrowser = nil; - - _deviceControllerDelegateBridge = new MTRDeviceControllerDelegateBridge(); - if ([self checkForInitError:(_deviceControllerDelegateBridge != nullptr) logMsg:kErrorPairingInit]) { - return nil; - } - - _partialDACVerifier = new chip::Credentials::PartialDACVerifier(); - if ([self checkForInitError:(_partialDACVerifier != nullptr) logMsg:kErrorPartialDacVerifierInit]) { - return nil; - } - - _operationalCredentialsDelegate = new MTROperationalCredentialsDelegate(self); - if ([self checkForInitError:(_operationalCredentialsDelegate != nullptr) logMsg:kErrorOperationalCredentialsInit]) { - return nil; - } - - // Provide a way to test different subscription pool sizes without code change - NSUserDefaults * defaults = [NSUserDefaults standardUserDefaults]; - if ([defaults objectForKey:kDefaultSubscriptionPoolSizeOverrideKey]) { - NSInteger subscriptionPoolSizeOverride = [defaults integerForKey:kDefaultSubscriptionPoolSizeOverrideKey]; - if (subscriptionPoolSizeOverride < 1) { - concurrentSubscriptionPoolSize = 1; - } else { - concurrentSubscriptionPoolSize = static_cast(subscriptionPoolSizeOverride); - } - - MTR_LOG(" *** Overriding pool size of MTRDeviceController with: %lu", static_cast(concurrentSubscriptionPoolSize)); - } - - if (!concurrentSubscriptionPoolSize) { - concurrentSubscriptionPoolSize = 1; - } - - MTR_LOG("%@ Setting up pool size of MTRDeviceController with: %lu", self, static_cast(concurrentSubscriptionPoolSize)); - - _concurrentSubscriptionPool = [[MTRAsyncWorkQueue alloc] initWithContext:self width:concurrentSubscriptionPoolSize]; - - _storedFabricIndex = chip::kUndefinedFabricIndex; - _storedCompressedFabricID = std::nullopt; - self.nodeID = nil; - self.fabricID = nil; - self.rootPublicKey = nil; - - _storageBehaviorConfiguration = storageBehaviorConfiguration; - } - return self; -} - - (NSString *)description { return [NSString stringWithFormat:@"<%@: %p, uuid: %@, suspended: %@>", NSStringFromClass(self.class), self, self.uniqueIdentifier, MTR_YES_NO(self.suspended)]; @@ -593,261 +438,6 @@ - (void)cleanup } } -- (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams -{ - __block BOOL commissionerInitialized = NO; - if ([self isRunning]) { - MTR_LOG_ERROR("%@ Unexpected duplicate call to startup", self); - return NO; - } - - dispatch_sync(_chipWorkQueue, ^{ - if ([self isRunning]) { - return; - } - - if (startupParams.vendorID == nil || [startupParams.vendorID unsignedShortValue] == chip::VendorId::Common) { - // Shouldn't be using the "standard" vendor ID for actual devices. - MTR_LOG_ERROR("%@ %@ is not a valid vendorID to initialize a device controller with", self, startupParams.vendorID); - return; - } - - if (startupParams.operationalCertificate == nil && startupParams.nodeID == nil) { - MTR_LOG_ERROR("%@ Can't start a controller if we don't know what node id it is", self); - return; - } - - if ([startupParams keypairsMatchCertificates] == NO) { - MTR_LOG_ERROR("%@ Provided keypairs do not match certificates", self); - return; - } - - if (startupParams.operationalCertificate != nil && startupParams.operationalKeypair == nil - && (!startupParams.fabricIndex.HasValue() - || !startupParams.keystore->HasOpKeypairForFabric(startupParams.fabricIndex.Value()))) { - MTR_LOG_ERROR("%@ Have no operational keypair for our operational certificate", self); - return; - } - - CHIP_ERROR errorCode = CHIP_ERROR_INCORRECT_STATE; - - // create a MTRP256KeypairBridge here and pass it to the operationalCredentialsDelegate - chip::Crypto::P256Keypair * signingKeypair = nullptr; - if (startupParams.nocSigner) { - errorCode = _signingKeypairBridge.Init(startupParams.nocSigner); - if ([self checkForStartError:errorCode logMsg:kErrorSigningKeypairInit]) { - return; - } - signingKeypair = &_signingKeypairBridge; - } - errorCode = _operationalCredentialsDelegate->Init( - signingKeypair, startupParams.ipk, startupParams.rootCertificate, startupParams.intermediateCertificate); - if ([self checkForStartError:errorCode logMsg:kErrorOperationalCredentialsInit]) { - return; - } - - _cppCommissioner = new chip::Controller::DeviceCommissioner(); - - // nocBuffer might not be used, but if it is it needs to live - // long enough (until after we are done using - // commissionerParams). - uint8_t nocBuffer[chip::Controller::kMaxCHIPDERCertLength]; - - chip::Controller::SetupParams commissionerParams; - - commissionerParams.pairingDelegate = _deviceControllerDelegateBridge; - - _operationalCredentialsDelegate->SetDeviceCommissioner(_cppCommissioner); - - commissionerParams.operationalCredentialsDelegate = _operationalCredentialsDelegate; - - commissionerParams.controllerRCAC = _operationalCredentialsDelegate->RootCertSpan(); - commissionerParams.controllerICAC = _operationalCredentialsDelegate->IntermediateCertSpan(); - - if (startupParams.operationalKeypair != nil) { - errorCode = _operationalKeypairBridge.Init(startupParams.operationalKeypair); - if ([self checkForStartError:errorCode logMsg:kErrorOperationalKeypairInit]) { - return; - } - commissionerParams.operationalKeypair = &_operationalKeypairBridge; - commissionerParams.hasExternallyOwnedOperationalKeypair = true; - } - - if (startupParams.operationalCertificate) { - commissionerParams.controllerNOC = AsByteSpan(startupParams.operationalCertificate); - } else { - chip::MutableByteSpan noc(nocBuffer); - - chip::CATValues cats = chip::kUndefinedCATs; - if (startupParams.caseAuthenticatedTags != nil) { - errorCode = SetToCATValues(startupParams.caseAuthenticatedTags, cats); - if (errorCode != CHIP_NO_ERROR) { - // SetToCATValues already handles logging. - return; - } - } - - if (commissionerParams.operationalKeypair != nullptr) { - errorCode = _operationalCredentialsDelegate->GenerateNOC(startupParams.nodeID.unsignedLongLongValue, - startupParams.fabricID.unsignedLongLongValue, cats, commissionerParams.operationalKeypair->Pubkey(), noc); - - if ([self checkForStartError:errorCode logMsg:kErrorGenerateNOC]) { - return; - } - } else { - // Generate a new random keypair. - uint8_t csrBuffer[chip::Crypto::kMIN_CSR_Buffer_Size]; - chip::MutableByteSpan csr(csrBuffer); - errorCode = startupParams.fabricTable->AllocatePendingOperationalKey(startupParams.fabricIndex, csr); - if ([self checkForStartError:errorCode logMsg:kErrorKeyAllocation]) { - return; - } - - chip::Crypto::P256PublicKey pubKey; - errorCode = VerifyCertificateSigningRequest(csr.data(), csr.size(), pubKey); - if ([self checkForStartError:errorCode logMsg:kErrorCSRValidation]) { - return; - } - - errorCode = _operationalCredentialsDelegate->GenerateNOC( - startupParams.nodeID.unsignedLongLongValue, startupParams.fabricID.unsignedLongLongValue, cats, pubKey, noc); - - if ([self checkForStartError:errorCode logMsg:kErrorGenerateNOC]) { - return; - } - } - commissionerParams.controllerNOC = noc; - } - commissionerParams.controllerVendorId = static_cast([startupParams.vendorID unsignedShortValue]); - commissionerParams.enableServerInteractions = startupParams.advertiseOperational; - - // 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 - // one, until something tells us otherwise. - const chip::Credentials::AttestationTrustStore * trustStore; - if (startupParams.productAttestationAuthorityCertificates) { - _attestationTrustStoreBridge - = new MTRAttestationTrustStoreBridge(startupParams.productAttestationAuthorityCertificates); - trustStore = _attestationTrustStoreBridge; - } else { - // TODO: Replace testingRootStore with a AttestationTrustStore that has the necessary official PAA roots available - trustStore = chip::Credentials::GetTestAttestationTrustStore(); - } - - _defaultDACVerifier = new chip::Credentials::DefaultDACVerifier(trustStore); - - if (startupParams.certificationDeclarationCertificates) { - auto cdTrustStore = _defaultDACVerifier->GetCertificationDeclarationTrustStore(); - if (cdTrustStore == nullptr) { - errorCode = CHIP_ERROR_INCORRECT_STATE; - } - if ([self checkForStartError:errorCode logMsg:kErrorCDCertStoreInit]) { - return; - } - - for (NSData * cdSigningCert in startupParams.certificationDeclarationCertificates) { - errorCode = cdTrustStore->AddTrustedKey(AsByteSpan(cdSigningCert)); - if ([self checkForStartError:errorCode logMsg:kErrorCDCertStoreInit]) { - return; - } - } - } - - commissionerParams.deviceAttestationVerifier = _defaultDACVerifier; - - auto & factory = chip::Controller::DeviceControllerFactory::GetInstance(); - - errorCode = factory.SetupCommissioner(commissionerParams, *_cppCommissioner); - if ([self checkForStartError:errorCode logMsg:kErrorCommissionerInit]) { - return; - } - - chip::FabricIndex fabricIdx = _cppCommissioner->GetFabricIndex(); - - uint8_t compressedIdBuffer[sizeof(uint64_t)]; - chip::MutableByteSpan compressedId(compressedIdBuffer); - errorCode = _cppCommissioner->GetCompressedFabricIdBytes(compressedId); - if ([self checkForStartError:errorCode logMsg:kErrorIPKInit]) { - return; - } - - errorCode = chip::Credentials::SetSingleIpkEpochKey( - _factory.groupDataProvider, fabricIdx, _operationalCredentialsDelegate->GetIPK(), compressedId); - if ([self checkForStartError:errorCode logMsg:kErrorIPKInit]) { - return; - } - - self->_storedFabricIndex = fabricIdx; - self->_storedCompressedFabricID = _cppCommissioner->GetCompressedFabricId(); - - chip::Crypto::P256PublicKey rootPublicKey; - if (_cppCommissioner->GetRootPublicKey(rootPublicKey) == CHIP_NO_ERROR) { - self.rootPublicKey = [NSData dataWithBytes:rootPublicKey.Bytes() length:rootPublicKey.Length()]; - self.nodeID = @(_cppCommissioner->GetNodeId()); - self.fabricID = @(_cppCommissioner->GetFabricId()); - } - - commissionerInitialized = YES; - - MTR_LOG("%@ startup succeeded for nodeID 0x%016llX", self, self->_cppCommissioner->GetNodeId()); - }); - - if (commissionerInitialized == NO) { - MTR_LOG_ERROR("%@ startup failed", self); - [self cleanupAfterStartup]; - return NO; - } - - // TODO: Once setNocChainIssuer no longer needs to be supported, - // we can just move the internals of - // setOperationalCertificateIssuer into the sync-dispatched block - // above. - if (![self setOperationalCertificateIssuer:startupParams.operationalCertificateIssuer - queue:startupParams.operationalCertificateIssuerQueue]) { - MTR_LOG_ERROR("%@ operationalCertificateIssuer and operationalCertificateIssuerQueue must both be nil or both be non-nil", self); - [self cleanupAfterStartup]; - return NO; - } - - if (_controllerDataStore) { - // If the storage delegate supports the bulk read API, then a dictionary of nodeID => cluster data dictionary would be passed to the handler. Otherwise this would be a no-op, and stored attributes for MTRDevice objects will be loaded lazily in -deviceForNodeID:. - [_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary *> * _Nonnull clusterDataByNode) { - MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast(clusterDataByNode.count), self->_uniqueIdentifier); - - std::lock_guard lock(*self.deviceMapLock); - NSMutableArray * deviceList = [NSMutableArray array]; - for (NSNumber * nodeID in clusterDataByNode) { - NSDictionary * clusterData = clusterDataByNode[nodeID]; - MTRDevice * device = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:clusterData]; - MTR_LOG("%@ Loaded %lu cluster data from storage for %@", self, static_cast(clusterData.count), device); - - [deviceList addObject:device]; - } - -#define kSecondsToWaitBeforeAPIClientRetainsMTRDevice 60 - // Keep the devices retained for a while, in case API client doesn't immediately retain them. - // - // Note that this is just an optimization to avoid throwing the information away and immediately - // re-reading it from storage. - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ - MTR_LOG("%@ un-retain devices loaded at startup %lu", self, static_cast(deviceList.count)); - }); - }]; - } - MTR_LOG("%@ startup: %@", NSStringFromClass(self.class), self); - - return YES; -} - - (NSNumber *)controllerNodeID { auto block = ^NSNumber * { return @(self->_cppCommissioner->GetNodeId()); }; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index c44b6e3bfb25dc..d052eef51d0414 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -461,7 +461,7 @@ - (void)stopControllerFactory * The provided controller is expected to have just been allocated and to not be * initialized yet. */ -- (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *)controller +- (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController_Concrete *)controller startupParams:(id)startupParams fabricChecker:(MTRDeviceControllerStartupParamsInternal * (^)(FabricTable * fabricTable, MTRDeviceController * controller, @@ -834,7 +834,7 @@ - (BOOL)findMatchingFabric:(FabricTable &)fabricTable // Returns nil on failure, the input controller on success. // If the provider has been initialized already, it is not considered as a failure. // -- (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceController * _Nonnull)controller +- (MTRDeviceController_Concrete * _Nullable)maybeInitializeOTAProvider:(MTRDeviceController_Concrete * _Nonnull)controller { [self _assertCurrentQueueIsNotMatterQueue]; @@ -1156,7 +1156,7 @@ - (nullable MTRDeviceController *)_findPendingShutdownControllerWithOperationalC return nil; } -- (nullable MTRDeviceController *)initializeController:(MTRDeviceController *)controller +- (nullable MTRDeviceController *)initializeController:(MTRDeviceController_Concrete *)controller withParameters:(MTRDeviceControllerParameters *)parameters error:(NSError * __autoreleasing *)error { diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index 7c59a65dba42c2..55c67ed8043c70 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -30,6 +30,7 @@ #import "MTRDefines_Internal.h" #import "MTRDeviceControllerFactory.h" +#import "MTRDeviceController_Concrete.h" #import "MTROperationalBrowser.h" #include @@ -89,9 +90,9 @@ MTR_DIRECT_MEMBERS completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion; /** - * Initialize an MTRDeviceController with the given parameters. + * Initialize an MTRDeviceController_Concrete with the given parameters. */ -- (nullable MTRDeviceController *)initializeController:(MTRDeviceController *)controller +- (nullable MTRDeviceController *)initializeController:(MTRDeviceController_Concrete *)controller withParameters:(MTRDeviceControllerParameters *)parameters error:(NSError * __autoreleasing *)error; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index 59dd42c04a97f4..615765fc84dae1 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -20,10 +20,40 @@ #import #import +#import "MTRDeviceControllerStartupParams_Internal.h" + NS_ASSUME_NONNULL_BEGIN @interface MTRDeviceController_Concrete : MTRDeviceController +/** + * Init a newly created controller. + * + * Only MTRDeviceControllerFactory should be calling this. + */ +- (nullable instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory + queue:(dispatch_queue_t)queue + storageDelegate:(id _Nullable)storageDelegate + storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue + otaProviderDelegate:(id _Nullable)otaProviderDelegate + otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue + uniqueIdentifier:(NSUUID *)uniqueIdentifier + concurrentSubscriptionPoolSize:(NSUInteger)concurrentSubscriptionPoolSize + storageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration + startSuspended:(BOOL)startSuspended; + +/** + * Start a new controller. Returns whether startup succeeded. If this fails, + * it guarantees that it has called controllerShuttingDown on the + * MTRDeviceControllerFactory. + * + * The return value will always match [controller isRunning] for this + * controller. + * + * Only MTRDeviceControllerFactory should be calling this. + */ +- (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams; + /** * Takes an assertion to keep the controller running. If `-[MTRDeviceController shutdown]` is called while an assertion * is held, the shutdown will be honored only after all assertions are released. Invoking this method multiple times increases diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 8784b61d63cb88..0c768b2ece0671 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -163,16 +163,16 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete return controller; } -- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory - queue:(dispatch_queue_t)queue - storageDelegate:(id _Nullable)storageDelegate - storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - otaProviderDelegate:(id _Nullable)otaProviderDelegate - otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue - uniqueIdentifier:(NSUUID *)uniqueIdentifier - concurrentSubscriptionPoolSize:(NSUInteger)concurrentSubscriptionPoolSize - storageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration - startSuspended:(BOOL)startSuspended +- (nullable instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory + queue:(dispatch_queue_t)queue + storageDelegate:(id _Nullable)storageDelegate + storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue + otaProviderDelegate:(id _Nullable)otaProviderDelegate + otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue + uniqueIdentifier:(NSUUID *)uniqueIdentifier + concurrentSubscriptionPoolSize:(NSUInteger)concurrentSubscriptionPoolSize + storageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration + startSuspended:(BOOL)startSuspended { if (self = [super initForSubclasses:startSuspended]) { // Make sure our storage is all set up to work as early as possible, diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index 8eb21aef64eb9e..d57a6e88a48d09 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -40,13 +40,11 @@ #import #import -#import #import #import #import @class MTRDeviceControllerParameters; -@class MTRDeviceControllerStartupParamsInternal; @class MTRDeviceControllerFactory; @class MTRDevice; @class MTRAsyncWorkQueue; @@ -79,18 +77,6 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - MTRDeviceControllerFactory methods -/** - * Start a new controller. Returns whether startup succeeded. If this fails, - * it guarantees that it has called controllerShuttingDown on the - * MTRDeviceControllerFactory. - * - * The return value will always match [controller isRunning] for this - * controller. - * - * Only MTRDeviceControllerFactory should be calling this. - */ -- (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams; - /** * Will return chip::kUndefinedFabricIndex if we do not have a fabric index. */ @@ -135,22 +121,6 @@ NS_ASSUME_NONNULL_BEGIN */ @property (nonatomic, retain, nullable) NSData * rootPublicKey; -/** - * Init a newly created controller. - * - * Only MTRDeviceControllerFactory should be calling this. - */ -- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory - queue:(dispatch_queue_t)queue - storageDelegate:(id _Nullable)storageDelegate - storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - otaProviderDelegate:(id _Nullable)otaProviderDelegate - otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue - uniqueIdentifier:(NSUUID *)uniqueIdentifier - concurrentSubscriptionPoolSize:(NSUInteger)concurrentSubscriptionPoolSize - storageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration - startSuspended:(BOOL)startSuspended; - /** * Check whether this controller is running on the given fabric, as represented * by the provided FabricTable and fabric index. The provided fabric table may From d3428a1397e559c7048fcb593ed0b2d08040f656 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 19 Sep 2024 22:46:43 -0400 Subject: [PATCH 6/6] Mark setupDeviceForNodeID: on MTRDeviceController as abstract. (#35679) This is overridden by the XPC and Concrete implementations. Also, there is no need to override deviceForNodeID and removeDevice in MTRDeviceController_Concrete, so those overrides are removed. --- .../Framework/CHIP/MTRDeviceController.mm | 38 +------------------ .../CHIP/MTRDeviceControllerParameters.h | 2 +- .../CHIP/MTRDeviceController_Concrete.mm | 24 ------------ .../CHIP/MTRDeviceController_Internal.h | 4 ++ 4 files changed, 7 insertions(+), 61 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index aaf1289f184f98..b8e454b49080c3 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -780,44 +780,10 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID return [[MTRBaseDevice alloc] initWithNodeID:nodeID controller:self]; } -// If prefetchedClusterData is not provided, load attributes individually from controller data store - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary *)prefetchedClusterData { - os_unfair_lock_assert_owner(self.deviceMapLock); - - MTRDevice * deviceToReturn = [[MTRDevice_Concrete alloc] initWithNodeID:nodeID controller:self]; - // If we're not running, don't add the device to our map. That would - // create a cycle that nothing would break. Just return the device, - // which will be in exactly the state it would be in if it were created - // while we were running and then we got shut down. - if ([self isRunning]) { - [_nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID]; - } - - if (prefetchedClusterData) { - if (prefetchedClusterData.count) { - [deviceToReturn setPersistedClusterData:prefetchedClusterData]; - } - } else if (_controllerDataStore) { - // Load persisted cluster data if they exist. - NSDictionary * clusterData = [_controllerDataStore getStoredClusterDataForNodeID:nodeID]; - MTR_LOG("%@ Loaded %lu cluster data from storage for %@", self, static_cast(clusterData.count), deviceToReturn); - if (clusterData.count) { - [deviceToReturn setPersistedClusterData:clusterData]; - } - } - - // TODO: Figure out how to get the device data as part of our bulk-read bits. - if (_controllerDataStore) { - auto * deviceData = [_controllerDataStore getStoredDeviceDataForNodeID:nodeID]; - if (deviceData.count) { - [deviceToReturn setPersistedDeviceData:deviceData]; - } - } - - [deviceToReturn setStorageBehaviorConfiguration:_storageBehaviorConfiguration]; - - return deviceToReturn; + MTR_ABSTRACT_METHOD(); + return nil; } - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h b/src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h index 171c91c889a493..c18a49ba3aaba6 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerParameters.h @@ -166,7 +166,7 @@ MTR_NEWLY_AVAILABLE /** * A controller created from this way will connect to a remote instance of an MTRDeviceController loaded in an XPC Service * - * @param xpcConnectionBlock The XPC Connection block that will return an NSXPCConnection to the indended listener. + * @param xpcConnectionBlock The XPC Connection block that will return an NSXPCConnection to the intended listener. * * @param uniqueIdentifier The unique id to assign to the controller. * diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 0c768b2ece0671..1ddc43f488ab11 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -1193,30 +1193,6 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N return deviceToReturn; } -- (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID -{ - std::lock_guard lock(*self.deviceMapLock); - MTRDevice * deviceToReturn = [self.nodeIDToDeviceMap objectForKey:nodeID]; - if (!deviceToReturn) { - deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil]; - } - - return deviceToReturn; -} - -- (void)removeDevice:(MTRDevice *)device -{ - std::lock_guard lock(*self.deviceMapLock); - auto * nodeID = device.nodeID; - MTRDevice * deviceToRemove = [self.nodeIDToDeviceMap objectForKey:nodeID]; - if (deviceToRemove == device) { - [deviceToRemove invalidate]; - [self.nodeIDToDeviceMap removeObjectForKey:nodeID]; - } else { - MTR_LOG_ERROR("%@ Error: Cannot remove device %p with nodeID %llu", self, device, nodeID.unsignedLongLongValue); - } -} - #ifdef DEBUG - (NSDictionary *)unitTestGetDeviceAttributeCounts { diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index d57a6e88a48d09..bcdcb70d10cfc2 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -268,6 +268,10 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Device-specific data and SDK access // DeviceController will act as a central repository for this opaque dictionary that MTRDevice manages - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID; +/** + * _setupDeviceForNodeID is a hook expected to be implemented by subclasses to + * actually allocate a device object of the right type. + */ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary *)prefetchedClusterData; - (void)removeDevice:(MTRDevice *)device;