Skip to content

Commit

Permalink
Sync up MTRDeviceController and MTRDeviceController_Concrete and actu…
Browse files Browse the repository at this point in the history 
…ally start using the latter.

Specific changes:

* Fix includes in MTRDeviceController_Concrete, since it uses std::optional and
  os_unfair_lock, and includes MTRDeviceController.h via
  MTRDeviceController_Concrete.h already (and has to, because of the
  inheritance).
* Copy the assertion counter machinery into MTRDeviceController_Concrete for
  now. This includes the storage of the variables that machinery relies on.
* Remove incorrect deviceMapLock @synthesize: this is just implemented by the
  superclass, no need to do anything with it in the subclass.
* Switch the places that are starting a non-XPC controller to create instances
  of MTRDeviceController_Concrete, not MTRDeviceController.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Sep 6, 2024
1 parent e96ddd9 commit aa94c75
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 23 deletions.
11 changes: 10 additions & 1 deletion src/darwin/Framework/CHIP/MTRDeviceController.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#import "MTRDeviceControllerLocalTestStorage.h"
#import "MTRDeviceControllerStartupParams.h"
#import "MTRDeviceControllerStartupParams_Internal.h"
#import "MTRDeviceController_Concrete.h"
#import "MTRDeviceController_XPC.h"
#import "MTRDevice_Concrete.h"
#import "MTRDevice_Internal.h"
Expand Down Expand Up @@ -82,6 +83,8 @@

#import <os/lock.h>

// 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";
Expand Down Expand Up @@ -176,7 +179,7 @@ - (nullable MTRDeviceController *)initWithParameters:(MTRDeviceControllerAbstrac
auto * controllerParameters = static_cast<MTRDeviceControllerParameters *>(parameters);

// MTRDeviceControllerFactory will auto-start in per-controller-storage mode if necessary
return [MTRDeviceControllerFactory.sharedInstance initializeController:self withParameters:controllerParameters error:error];
return [MTRDeviceControllerFactory.sharedInstance initializeController:[MTRDeviceController_Concrete alloc] withParameters:controllerParameters error:error];
}

- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
Expand Down Expand Up @@ -1718,6 +1721,9 @@ + (void)forceLocalhostAdvertisingOnly

@end

// TODO: This should not be in the superclass: either move to
// MTRDeviceController_Concrete.mm, or move into a separate .h/.mm pair of
// files.
@implementation MTRDevicePairingDelegateShim
- (instancetype)initWithDelegate:(id<MTRDevicePairingDelegate>)delegate
{
Expand Down Expand Up @@ -1770,6 +1776,9 @@ - (void)onPairingDeleted:(NSError * _Nullable)error
* Shim to allow us to treat an MTRNOCChainIssuer as an
* MTROperationalCertificateIssuer.
*/
// TODO: This should not be in the superclass: either move to
// MTRDeviceController_Concrete.mm, or move into a separate .h/.mm pair of
// files.
@interface MTROperationalCertificateChainIssuerShim : NSObject <MTROperationalCertificateIssuer>
@property (nonatomic, readonly) id<MTRNOCChainIssuer> nocChainIssuer;
@property (nonatomic, readonly) BOOL shouldSkipAttestationCertificateValidation;
Expand Down
5 changes: 3 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#import "MTRDeviceController.h"
#import "MTRDeviceControllerStartupParams.h"
#import "MTRDeviceControllerStartupParams_Internal.h"
#import "MTRDeviceController_Concrete.h"
#import "MTRDeviceController_Internal.h"
#import "MTRDiagnosticLogsDownloader.h"
#import "MTRError_Internal.h"
Expand Down Expand Up @@ -669,7 +670,7 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo
return existingController;
}

return [self _startDeviceController:[MTRDeviceController alloc]
return [self _startDeviceController:[MTRDeviceController_Concrete alloc]
startupParams:startupParams
fabricChecker:^MTRDeviceControllerStartupParamsInternal *(
FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) {
Expand Down Expand Up @@ -741,7 +742,7 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl
return nil;
}

return [self _startDeviceController:[MTRDeviceController alloc]
return [self _startDeviceController:[MTRDeviceController_Concrete alloc]
startupParams:startupParams
fabricChecker:^MTRDeviceControllerStartupParamsInternal *(
FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) {
Expand Down
130 changes: 110 additions & 20 deletions src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#import "MTRCommissionableBrowserResult_Internal.h"
#import "MTRCommissioningParameters.h"
#import "MTRConversion.h"
#import "MTRDeviceController.h"
#import "MTRDeviceControllerDelegateBridge.h"
#import "MTRDeviceControllerFactory_Internal.h"
#import "MTRDeviceControllerLocalTestStorage.h"
Expand All @@ -50,6 +49,7 @@
#import "MTRSetupPayload.h"
#import "MTRTimeUtils.h"
#import "MTRUnfairLock.h"
#import "MTRUtilities.h"
#import "NSDataSpanConversion.h"
#import "NSStringSpanConversion.h"
#import <setup_payload/ManualSetupPayloadGenerator.h>
Expand Down Expand Up @@ -80,8 +80,11 @@

#include <atomic>
#include <dns_sd.h>
#include <optional>
#include <string>

#import <os/lock.h>

typedef void (^SyncWorkQueueBlock)(void);
typedef id (^SyncWorkQueueBlockWithReturnValue)(void);
typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void);
Expand Down Expand Up @@ -114,19 +117,29 @@ @interface MTRDeviceController_Concrete ()
@end

@implementation MTRDeviceController_Concrete {
// queue used to serialize all work performed by the MTRDeviceController
std::atomic<chip::FabricIndex> _storedFabricIndex;
std::atomic<std::optional<uint64_t>> _storedCompressedFabricID;
MTRP256KeypairBridge _signingKeypairBridge;
MTRP256KeypairBridge _operationalKeypairBridge;

// Counters to track assertion status and access controlled by the _assertionLock
// TODO: Figure out whether they should live here or in the base class (or
// go away completely!), which depends on how the shutdown codepaths get set up.
NSUInteger _keepRunningAssertionCounter;
BOOL _shutdownPending;
os_unfair_lock _assertionLock;
}

// MTRDeviceController ivar internal access
// TODO: Figure out whether uniqueIdentifier storage should live in the superclass. It
// probably should!
@synthesize uniqueIdentifier = _uniqueIdentifier;
// TODO: Figure out whether the work queue storage lives here or in the superclass
// Right now we seem to have both?
@synthesize chipWorkQueue = _chipWorkQueue;
@synthesize controllerDataStore = _controllerDataStore;
// TODO: For these remaining ivars, figure out whether they should live here or
// on the superclass. Should not be both.
@synthesize factory = _factory;
@synthesize deviceMapLock = _deviceMapLock;
@synthesize otaProviderDelegate = _otaProviderDelegate;
@synthesize otaProviderDelegateQueue = _otaProviderDelegateQueue;
@synthesize commissionableBrowser = _commissionableBrowser;
Expand Down Expand Up @@ -165,7 +178,7 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete
MTR_LOG_DEBUG("%s: got standard parameters, getting standard device controller from factory", __PRETTY_FUNCTION__);
auto * controllerParameters = static_cast<MTRDeviceControllerParameters *>(parameters);

// or, if necessary, MTRDeviceControllerFactory will auto-start in per-controller-storage mode if necessary
// 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
Expand Down Expand Up @@ -196,6 +209,12 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
// 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;

// Setup assertion variables
_keepRunningAssertionCounter = 0;
_shutdownPending = NO;
_assertionLock = OS_UNFAIR_LOCK_INIT;

if (storageDelegate != nil) {
if (storageDelegateQueue == nil) {
MTR_LOG_ERROR("storageDelegate provided without storageDelegateQueue");
Expand Down Expand Up @@ -269,6 +288,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
_otaProviderDelegateQueue = otaProviderDelegateQueue;
_chipWorkQueue = queue;
_factory = factory;
// TODO: Shouldn't nodeIDToDeviceMap just be set up by initForSubclasses?
self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
_serverEndpoints = [[NSMutableArray alloc] init];
_commissionableBrowser = nil;
Expand Down Expand Up @@ -310,6 +330,10 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
_concurrentSubscriptionPool = [[MTRAsyncWorkQueue alloc] initWithContext:self width:concurrentSubscriptionPoolSize];

_storedFabricIndex = chip::kUndefinedFabricIndex;
_storedCompressedFabricID = std::nullopt;
self.nodeID = nil;
self.fabricID = nil;
self.rootPublicKey = nil;

_storageBehaviorConfiguration = storageBehaviorConfiguration;
}
Expand All @@ -326,8 +350,68 @@ - (BOOL)isRunning
return _cppCommissioner != nullptr;
}

- (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<unsigned long>(_keepRunningAssertionCounter));
}
}

- (void)removeRunAssertion;
{
std::lock_guard lock(_assertionLock);

if (_keepRunningAssertionCounter > 0) {
--_keepRunningAssertionCounter;
MTR_LOG("%@ Removing keep running assertion, total %lu", self, static_cast<unsigned long>(_keepRunningAssertionCounter));

if ([self isRunning] && _keepRunningAssertionCounter == 0 && _shutdownPending) {
MTR_LOG("%@ All assertions removed and shutdown is pending, shutting down", self);
[self finalShutdown];
}
}
}

- (void)clearPendingShutdown
{
std::lock_guard lock(_assertionLock);
_shutdownPending = NO;
}

- (void)shutdown
{
std::lock_guard lock(_assertionLock);

if (_keepRunningAssertionCounter > 0) {
MTR_LOG("%@ Pending shutdown since %lu assertions are present", self, static_cast<unsigned long>(_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.
Expand Down Expand Up @@ -383,11 +467,17 @@ - (void)shutDownCppController
// shutdown completes, in case it wants to write to storage as it
// shuts down.
_storedFabricIndex = chip::kUndefinedFabricIndex;
_storedCompressedFabricID = std::nullopt;
self.nodeID = nil;
self.fabricID = nil;
self.rootPublicKey = nil;

delete commissionerToShutDown;
if (_operationalCredentialsDelegate != nil) {
_operationalCredentialsDelegate->SetDeviceCommissioner(nullptr);
}
}
_shutdownPending = NO;
}

- (void)deinitFromFactory
Expand Down Expand Up @@ -622,6 +712,15 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
}

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());
Expand Down Expand Up @@ -669,7 +768,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
});
}];
}
MTR_LOG("%s: startup: %@", __PRETTY_FUNCTION__, self);
MTR_LOG("%@ startup: %@", NSStringFromClass(self.class), self);

return YES;
}
Expand Down Expand Up @@ -1279,6 +1378,9 @@ - (BOOL)checkForStartError:(CHIP_ERROR)errorCode logMsg:(NSString *)logMsg
return YES;
}

// TODO: Figure out whether this should live here or in superclass; we shouldn't
// have two copies of this thing. Probably after removing code from the
// superclass that should not be there.
+ (BOOL)checkForError:(CHIP_ERROR)errorCode logMsg:(NSString *)logMsg error:(NSError * __autoreleasing *)error
{
if (CHIP_NO_ERROR == errorCode) {
Expand Down Expand Up @@ -1472,20 +1574,8 @@ - (BOOL)syncRunOnWorkQueueWithBoolReturnValue:(SyncWorkQueueBlockWithBoolReturnV

- (nullable NSNumber *)compressedFabricID
{
assertChipStackLockedByCurrentThread();

if (!_cppCommissioner) {
return nil;
}

return @(_cppCommissioner->GetCompressedFabricId());
}

- (NSNumber * _Nullable)syncGetCompressedFabricID
{
return [self syncRunOnWorkQueueWithReturnValue:^NSNumber * {
return [self compressedFabricID];
} error:nil];
auto storedValue = _storedCompressedFabricID.load();
return storedValue.has_value() ? @(storedValue.value()) : nil;
}

- (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
Expand Down

0 comments on commit aa94c75

Please sign in to comment.