Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Darwin: MTRDeviceControllerFactory improvements #32997

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Darwin: MTRDeviceControllerFactory improvements #32997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d95bbd
Darwin: MTRDeviceControllerFactory improvements
ksperling-apple Apr 16, 2024
d21c082
Darwin: OOM handling at the framework level is unnecessary
ksperling-apple Apr 16, 2024
17c2801
Lint
ksperling-apple Apr 16, 2024
9723169
Use copy instead of dataWithData
ksperling-apple Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1795,11 +1795,7 @@ static CHIP_ERROR OpenCommissioningWindow(Controller::DeviceController * control
CHIP_ERROR OpenCommissioningWindowHelper::OpenCommissioningWindow(Controller::DeviceController * controller, NodeId nodeID,
System::Clock::Seconds16 timeout, uint16_t discriminator, const Optional<uint32_t> & setupPIN, ResultCallback callback)
{
auto * self = new (std::nothrow) OpenCommissioningWindowHelper(controller, callback);
if (self == nullptr) {
return CHIP_ERROR_NO_MEMORY;
}

auto * self = new OpenCommissioningWindowHelper(controller, callback);
SetupPayload unused;
CHIP_ERROR err = self->mOpener.OpenCommissioningWindow(nodeID, timeout, Crypto::kSpake2p_Min_PBKDF_Iterations, discriminator,
setupPIN, NullOptional, &self->mOnOpenCommissioningWindowCallback, unused);
Expand Down
4 changes: 0 additions & 4 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,10 +413,6 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
}

_cppCommissioner = new chip::Controller::DeviceCommissioner();
if (_cppCommissioner == nullptr) {
[self checkForStartError:CHIP_ERROR_NO_MEMORY logMsg:kErrorCommissionerInit];
return;
}

// nocBuffer might not be used, but if it is it needs to live
// long enough (until after we are done using
Expand Down
179 changes: 52 additions & 127 deletions src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,6 @@
using namespace chip;
using namespace chip::Controller;

static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate";
static NSString * const kErrorSessionResumptionStorageInit = @"Init failure while creating a session resumption storage delegate";
static NSString * const kErrorControllerFactoryInit = @"Init failure while initializing controller factory";
static NSString * const kErrorKeystoreInit = @"Init failure while initializing persistent storage keystore";
static NSString * const kErrorCertStoreInit = @"Init failure while initializing persistent storage operational certificate store";

static bool sExitHandlerRegistered = false;
static void ShutdownOnExit()
{
Expand Down Expand Up @@ -228,21 +222,6 @@ - (void)_assertCurrentQueueIsNotMatterQueue
VerifyOrDie(!DeviceLayer::PlatformMgrImpl().IsWorkQueueCurrentQueue());
}

- (BOOL)checkIsRunning:(NSError * __autoreleasing *)error
{
[self _assertCurrentQueueIsNotMatterQueue];

if ([self isRunning]) {
return YES;
}

if (error != nil) {
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE];
}

return NO;
}

- (void)cleanupStartupObjects
{
assertChipStackLockedByCurrentThread();
Expand Down Expand Up @@ -293,7 +272,7 @@ - (CHIP_ERROR)_initFabricTable:(FabricTable &)fabricTable
{
[self _assertCurrentQueueIsNotMatterQueue];

if (!self.isRunning) {
if (!_running) { // Note: reading _running from outside of the Matter work queue
return nil;
}

Expand Down Expand Up @@ -339,40 +318,25 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam
{
[self _assertCurrentQueueIsNotMatterQueue];

if ([self isRunning]) {
MTR_LOG_DEBUG("Ignoring duplicate call to startup, Matter controller factory already started...");
return YES;
}

__block CHIP_ERROR errorCode = CHIP_NO_ERROR;
__block CHIP_ERROR err = CHIP_ERROR_INTERNAL;
dispatch_sync(_chipWorkQueue, ^{
if ([self isRunning]) {
return;
if (_running) {
// TODO: When treating a duplicate call as success we should validate parameters match
MTR_LOG_DEBUG("Ignoring duplicate call to startup, Matter controller factory already started...");
ExitNow(err = CHIP_NO_ERROR);
}

StartupMetricsCollection();
InitializeServerAccessControl();

if (startupParams.hasStorage) {
_persistentStorageDelegate = new (std::nothrow) MTRPersistentStorageDelegateBridge(startupParams.storage);
_persistentStorageDelegate = new MTRPersistentStorageDelegateBridge(startupParams.storage);
_sessionResumptionStorage = nullptr;
_usingPerControllerStorage = NO;
} else {
_persistentStorageDelegate = new (std::nothrow) MTRDemuxingStorage(self);
_sessionResumptionStorage = new (std::nothrow) MTRSessionResumptionStorageBridge(self);
_persistentStorageDelegate = new MTRDemuxingStorage(self);
_sessionResumptionStorage = new MTRSessionResumptionStorageBridge(self);
_usingPerControllerStorage = YES;

if (_sessionResumptionStorage == nil) {
MTR_LOG_ERROR("Error: %@", kErrorSessionResumptionStorageInit);
errorCode = CHIP_ERROR_NO_MEMORY;
return;
}
}

if (_persistentStorageDelegate == nil) {
MTR_LOG_ERROR("Error: %@", kErrorPersistentStorageInit);
errorCode = CHIP_ERROR_NO_MEMORY;
return;
}

_otaProviderDelegate = startupParams.otaProviderDelegate;
Expand All @@ -383,63 +347,42 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam

// TODO: Allow passing a different keystore implementation via startupParams.
_keystore = new PersistentStorageOperationalKeystore();
if (_keystore == nullptr) {
MTR_LOG_ERROR("Error: %@", kErrorKeystoreInit);
errorCode = CHIP_ERROR_NO_MEMORY;
return;
}

errorCode = _keystore->Init(_persistentStorageDelegate);
if (errorCode != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Error: %@", kErrorKeystoreInit);
return;
}
SuccessOrExit(err = _keystore->Init(_persistentStorageDelegate));

// TODO Allow passing a different opcert store implementation via startupParams.
_opCertStore = new Credentials::PersistentStorageOpCertStore();
if (_opCertStore == nullptr) {
MTR_LOG_ERROR("Error: %@", kErrorCertStoreInit);
errorCode = CHIP_ERROR_NO_MEMORY;
return;
}

errorCode = _opCertStore->Init(_persistentStorageDelegate);
if (errorCode != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Error: %@", kErrorCertStoreInit);
return;
}
SuccessOrExit(err = _opCertStore->Init(_persistentStorageDelegate));

_productAttestationAuthorityCertificates = [startupParams.productAttestationAuthorityCertificates copy];
_certificationDeclarationCertificates = [startupParams.certificationDeclarationCertificates copy];

chip::Controller::FactoryInitParams params;
if (startupParams.port != nil) {
params.listenPort = [startupParams.port unsignedShortValue];
}
params.enableServerInteractions = startupParams.shouldStartServer;

params.groupDataProvider = &_groupDataProvider;
params.sessionKeystore = &_sessionKeystore;
params.fabricIndependentStorage = _persistentStorageDelegate;
params.operationalKeystore = _keystore;
params.opCertStore = _opCertStore;
params.certificateValidityPolicy = &_certificateValidityPolicy;
params.sessionResumptionStorage = _sessionResumptionStorage;
errorCode = _controllerFactory->Init(params);
if (errorCode != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Error: %@", kErrorControllerFactoryInit);
return;
{
chip::Controller::FactoryInitParams params;
if (startupParams.port != nil) {
params.listenPort = [startupParams.port unsignedShortValue];
}
params.enableServerInteractions = startupParams.shouldStartServer;

params.groupDataProvider = &_groupDataProvider;
params.sessionKeystore = &_sessionKeystore;
params.fabricIndependentStorage = _persistentStorageDelegate;
params.operationalKeystore = _keystore;
params.opCertStore = _opCertStore;
params.certificateValidityPolicy = &_certificateValidityPolicy;
params.sessionResumptionStorage = _sessionResumptionStorage;
SuccessOrExit(err = _controllerFactory->Init(params));
}

// This needs to happen after DeviceControllerFactory::Init,
// because that creates (lazily, by calling functions with
// static variables in them) some static-lifetime objects.
if (!sExitHandlerRegistered) {
int ret = atexit(ShutdownOnExit);
if (ret != 0) {
MTR_LOG_ERROR("Error registering exit handler: %d", ret);
return;
if (atexit(ShutdownOnExit) != 0) {
char error[128];
strerror_r(errno, error, sizeof(error));
MTR_LOG_ERROR("Warning: Failed to register atexit handler: %s", error);
}
sExitHandlerRegistered = true;
ksperling-apple marked this conversation as resolved.
Show resolved Hide resolved
}
HeapObjectPoolExitHandling::IgnoreLeaksOnExit();

Expand All @@ -452,47 +395,44 @@ - (BOOL)_startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParam
_controllerFactory->RetainSystemState();
_controllerFactory->ReleaseSystemState();

self->_advertiseOperational = startupParams.shouldStartServer;
self->_running = YES;
});
_advertiseOperational = startupParams.shouldStartServer;
_running = YES;
err = CHIP_NO_ERROR;

if (![self isRunning]) {
dispatch_sync(_chipWorkQueue, ^{
exit:
if (err != CHIP_NO_ERROR) {
// Note: Since we have failed no later than _controllerFactory->Init(),
// there is no need to call _controllerFactory->Shutdown() here.
[self cleanupStartupObjects];
});
}
});
if (err != CHIP_NO_ERROR) {
MTR_LOG_ERROR("Failed to start Matter controller factory: %" CHIP_ERROR_FORMAT, err.Format());
if (error != nil) {
*error = [MTRError errorForCHIPErrorCode:errorCode];
*error = [MTRError errorForCHIPErrorCode:err];
}
return NO;
}

return YES;
}

- (void)stopControllerFactory
{
[self _assertCurrentQueueIsNotMatterQueue];

if (![self isRunning]) {
ksperling-apple marked this conversation as resolved.
Show resolved Hide resolved
return;
}

while ([_controllers count] != 0) {
[_controllers[0] shutdown];
}

dispatch_sync(_chipWorkQueue, ^{
VerifyOrReturn(_running);

MTR_LOG_INFO("Shutting down the Matter controller factory");
_controllerFactory->Shutdown();

[self cleanupStartupObjects];
_running = NO;
_advertiseOperational = NO;
});

// NOTE: we do not call cleanupInitObjects because we can be restarted, and
// that does not re-create the objects that we create inside init.
// Maybe we should be creating them in startup?

_running = NO;
}

/**
Expand Down Expand Up @@ -540,7 +480,7 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *
return nil;
}

if (![self isRunning]) {
if (!_running) { // Note: reading _running from outside of the Matter work queue
if (storageDelegate != nil) {
MTR_LOG_DEFAULT("Auto-starting Matter controller factory in per-controller storage mode");
auto * params = [[MTRDeviceControllerFactoryParams alloc] initWithoutStorage];
Expand Down Expand Up @@ -744,13 +684,8 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo
keystore:self->_keystore
advertiseOperational:self->_advertiseOperational
params:startupParams];
if (params == nil) {
fabricError = CHIP_ERROR_NO_MEMORY;
} else {
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
}

params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
return params;
}
error:error];
Expand Down Expand Up @@ -800,12 +735,8 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl
keystore:self->_keystore
advertiseOperational:self->_advertiseOperational
params:startupParams];
if (params == nil) {
fabricError = CHIP_ERROR_NO_MEMORY;
} else {
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
}
params.productAttestationAuthorityCertificates = self->_productAttestationAuthorityCertificates;
params.certificationDeclarationCertificates = self->_certificationDeclarationCertificates;
return params;
}
error:error];
Expand Down Expand Up @@ -907,12 +838,6 @@ - (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceControll

- (void)resetOperationalAdvertising
{
if (![self checkIsRunning:nil]) {
woody-apple marked this conversation as resolved.
Show resolved Hide resolved
// No need to reset anything; we are not running, so not
// advertising.
return;
}

if (!_advertiseOperational) {
// No need to reset anything; we are not advertising the things that
// would need to get reset.
Expand Down
18 changes: 5 additions & 13 deletions src/darwin/Framework/CHIP/MTRManualSetupPayloadParser.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,11 @@ - (MTRSetupPayload *)populatePayload:(NSError * __autoreleasing *)error
chip::SetupPayload cPlusPluspayload;
MTRSetupPayload * payload;

if (_chipManualSetupPayloadParser) {
CHIP_ERROR chipError = _chipManualSetupPayloadParser->populatePayload(cPlusPluspayload);

if (chipError == CHIP_NO_ERROR) {
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
} else if (error) {
*error = [MTRError errorForCHIPErrorCode:chipError];
}
} else {
// Memory init has failed
if (error) {
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY];
}
CHIP_ERROR chipError = _chipManualSetupPayloadParser->populatePayload(cPlusPluspayload);
if (chipError == CHIP_NO_ERROR) {
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
} else if (error) {
*error = [MTRError errorForCHIPErrorCode:chipError];
}

return payload;
Expand Down
13 changes: 2 additions & 11 deletions src/darwin/Framework/CHIP/MTROperationalCredentialsDelegate.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,8 @@

// Make copies of the certificates, just in case the API consumer
// has them as MutableData.
mRootCert = [NSData dataWithData:rootCert];
if (mRootCert == nil) {
ksperling-apple marked this conversation as resolved.
Show resolved Hide resolved
return CHIP_ERROR_NO_MEMORY;
}

if (icaCert != nil) {
mIntermediateCert = [NSData dataWithData:icaCert];
if (mIntermediateCert == nil) {
ksperling-apple marked this conversation as resolved.
Show resolved Hide resolved
return CHIP_ERROR_NO_MEMORY;
}
}
mRootCert = [rootCert copy];
mIntermediateCert = [icaCert copy];

return CHIP_NO_ERROR;
}
Expand Down
18 changes: 5 additions & 13 deletions src/darwin/Framework/CHIP/MTRQRCodeSetupPayloadParser.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,11 @@ - (MTRSetupPayload *)populatePayload:(NSError * __autoreleasing *)error
chip::SetupPayload cPlusPluspayload;
MTRSetupPayload * payload;

if (_chipQRCodeSetupPayloadParser) {
CHIP_ERROR chipError = _chipQRCodeSetupPayloadParser->populatePayload(cPlusPluspayload);

if (chipError == CHIP_NO_ERROR) {
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
} else if (error) {
*error = [MTRError errorForCHIPErrorCode:chipError];
}
} else {
// Memory init has failed
if (error) {
*error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY];
}
CHIP_ERROR chipError = _chipQRCodeSetupPayloadParser->populatePayload(cPlusPluspayload);
if (chipError == CHIP_NO_ERROR) {
payload = [[MTRSetupPayload alloc] initWithSetupPayload:cPlusPluspayload];
} else if (error) {
*error = [MTRError errorForCHIPErrorCode:chipError];
}

return payload;
Expand Down
Loading
Loading