From 1419084cd313d39f27d400c42db51107a6f32f12 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 1 Nov 2022 17:05:32 -0400 Subject: [PATCH] Address Darwin API review comments on MTRThreadOperationalDataset. (#23392) * Address Darwin API review comments on MTRThreadOperationalDataset. * Improve documentation * Actually use our size constants in the error-checks we do. * Make the properties non-nullable, since we always have them after successful init. This is a re-landing of PR #22562 but modified to preserve old APIs. * Address review comment. --- .../CHIP/MTRThreadOperationalDataset.h | 57 ++++++++++++------- .../CHIP/MTRThreadOperationalDataset.mm | 36 ++++++------ .../MTRThreadOperationalDatasetTests.mm | 2 +- 3 files changed, 55 insertions(+), 40 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.h b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.h index 5c8bb60185086d..fa2e6b73975ea6 100644 --- a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.h +++ b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.h @@ -19,52 +19,66 @@ NS_ASSUME_NONNULL_BEGIN +/** + * MTRThreadOperationalDataset allows converting between an "expanded" view of + * the dataset (with the separate fields) and a single-blob NSData view. + * + * The latter can be used to pass Thread network credentials via + * MTRCommissioningParameters. + */ @interface MTRThreadOperationalDataset : NSObject /** - * The expected lengths of each of the NSData fields in the CHIPThreadOperationalDataset - * - * initWithNetworkName must be provided NSData fields with at least these lengths otherwise - * the object will fail to init. + * The expected lengths of each of the NSData fields in the MTRThreadOperationalDataset */ extern size_t const MTRSizeThreadNetworkName; -extern size_t const MTRSizeThreadExtendedPanId; +extern size_t const MTRSizeThreadExtendedPanId MTR_NEWLY_DEPRECATED("Please use MTRSizeThreadExtendedPANID"); +extern size_t const MTRSizeThreadExtendedPANID MTR_NEWLY_AVAILABLE; extern size_t const MTRSizeThreadMasterKey; extern size_t const MTRSizeThreadPSKc; +extern size_t const MTRSizeThreadPANID MTR_NEWLY_AVAILABLE; /** - * The Thread Network name + * The Thread Network name */ -@property (nonatomic, nullable, copy, readonly) NSString * networkName; +@property (nonatomic, copy, readonly) NSString * networkName; /** - * The Thread Network extendended PAN ID + * The Thread Network extendended PAN ID */ -@property (nonatomic, nullable, copy, readonly) NSData * extendedPANID; +@property (nonatomic, copy, readonly) NSData * extendedPANID; /** - * The 16 byte Master Key + * The 16 byte Master Key */ -@property (nonatomic, nullable, copy, readonly) NSData * masterKey; +@property (nonatomic, copy, readonly) NSData * masterKey; /** - * The Thread PSKc + * The Thread PSKc */ -@property (nonatomic, nullable, copy, readonly) NSData * PSKc; +@property (nonatomic, copy, readonly) NSData * PSKc; /** - * The Thread network channel. Always an unsigned 16-bit integer. + * The Thread network channel. Always an unsigned 16-bit integer. */ @property (nonatomic, copy, readonly) NSNumber * channelNumber MTR_NEWLY_AVAILABLE; /** - * A uint16_t stored as 2-bytes in host order representing the Thread PAN ID + * A uint16_t stored as 2-bytes in host order representing the Thread PAN ID */ -@property (nonatomic, nullable, copy, readonly) NSData * panID; +@property (nonatomic, copy, readonly) NSData * panID; - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; /** - * Create a Thread Operational Dataset object with the individual network fields. - * This initializer will return nil if any of the NSData fields don't match the expected size. + * Create a Thread Operational Dataset object with the individual network + * fields. * - * Note: The panID is expected to be a uint16_t stored as 2-bytes in host order + * @param extendedPANID Must be MTRSizeThreadExtendedPANID bytes. Otherwise nil + * will be returned. + * @param masterKey Must be MTRSizeThreadMasterKey bytes. Otherwise nil will be + * returned. + * @param PSKc Must be MTRSizeThreadPSKc bytes. Otherwise nil will be returned. + * @param channelNumber Must be an unsigned 16-bit value. + * @param panID Must be MTRSizeThreadPANID bytes. Otherwise nil will be + * returned. In particular, it's expected to be a 16-bit unsigned + * integer stored as 2 bytes in host order. */ - (nullable instancetype)initWithNetworkName:(NSString *)networkName extendedPANID:(NSData *)extendedPANID @@ -74,13 +88,14 @@ extern size_t const MTRSizeThreadPSKc; panID:(NSData *)panID MTR_NEWLY_AVAILABLE; /** - * Create a Thread Operational Dataset object with a RCP formatted active operational dataset. - * This initializer will return nil if the input data cannot be parsed correctly + * Create a Thread Operational Dataset object with a RCP formatted active operational dataset. + * This initializer will return nil if the input data cannot be parsed correctly */ - (nullable instancetype)initWithData:(NSData *)data; /** * Get the underlying data that represents the Thread Active Operational Dataset + * This can be used for the threadOperationalDataset of MTRCommissioningParameters. */ - (NSData *)data; diff --git a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm index fe8a6d003ca74c..d8831cec2cfb15 100644 --- a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm +++ b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm @@ -22,9 +22,11 @@ #include size_t const MTRSizeThreadNetworkName = chip::Thread::kSizeNetworkName; -size_t const MTRSizeThreadExtendedPanId = chip::Thread::kSizeExtendedPanId; +size_t const MTRSizeThreadExtendedPANID = chip::Thread::kSizeExtendedPanId; +size_t const MTRSizeThreadExtendedPanId = MTRSizeThreadExtendedPANID; size_t const MTRSizeThreadMasterKey = chip::Thread::kSizeMasterKey; size_t const MTRSizeThreadPSKc = chip::Thread::kSizePSKc; +size_t const MTRSizeThreadPANID = 2; // Thread's PAN ID is 2 bytes @interface MTRThreadOperationalDataset () @@ -62,43 +64,41 @@ - (BOOL)_populateCppOperationalDataset _cppThreadOperationalDataset.Clear(); _cppThreadOperationalDataset.SetNetworkName([self.networkName cStringUsingEncoding:NSUTF8StringEncoding]); - if (![self _checkDataLength:self.extendedPANID expectedLength:chip::Thread::kSizeExtendedPanId]) { + if (![self _checkDataLength:self.extendedPANID expectedLength:MTRSizeThreadExtendedPANID]) { MTR_LOG_ERROR("Invalid ExtendedPANID"); return NO; } - uint8_t extendedPanId[chip::Thread::kSizeExtendedPanId]; - [self.extendedPANID getBytes:&extendedPanId length:chip::Thread::kSizeExtendedPanId]; + uint8_t extendedPanId[MTRSizeThreadExtendedPANID]; + [self.extendedPANID getBytes:&extendedPanId length:MTRSizeThreadExtendedPANID]; _cppThreadOperationalDataset.SetExtendedPanId(extendedPanId); - if (![self _checkDataLength:self.masterKey expectedLength:chip::Thread::kSizeMasterKey]) { + if (![self _checkDataLength:self.masterKey expectedLength:MTRSizeThreadMasterKey]) { MTR_LOG_ERROR("Invalid MasterKey"); return NO; } - uint8_t masterKey[chip::Thread::kSizeMasterKey]; - [self.masterKey getBytes:&masterKey length:chip::Thread::kSizeMasterKey]; + uint8_t masterKey[MTRSizeThreadMasterKey]; + [self.masterKey getBytes:&masterKey length:MTRSizeThreadMasterKey]; _cppThreadOperationalDataset.SetMasterKey(masterKey); - if (![self _checkDataLength:self.PSKc expectedLength:chip::Thread::kSizePSKc]) { + if (![self _checkDataLength:self.PSKc expectedLength:MTRSizeThreadPSKc]) { MTR_LOG_ERROR("Invalid PKSc"); return NO; } - uint8_t PSKc[chip::Thread::kSizePSKc]; - [self.PSKc getBytes:&PSKc length:chip::Thread::kSizePSKc]; + uint8_t PSKc[MTRSizeThreadPSKc]; + [self.PSKc getBytes:&PSKc length:MTRSizeThreadPSKc]; _cppThreadOperationalDataset.SetPSKc(PSKc); _cppThreadOperationalDataset.SetChannel([self.channelNumber unsignedShortValue]); // Thread's PAN ID is 2 bytes - if (![self _checkDataLength:self.panID expectedLength:2]) { + if (![self _checkDataLength:self.panID expectedLength:MTRSizeThreadPANID]) { MTR_LOG_ERROR("Invalid PAN ID"); return NO; } - uint16_t * valuePtr = (uint16_t *) [self.panID bytes]; - if (valuePtr == nullptr) { - return NO; - } + uint16_t panID; + memcpy(&panID, [self.panID bytes], MTRSizeThreadPANID); // The underlying CPP class assumes Big Endianness for the panID - _cppThreadOperationalDataset.SetPanId(CFSwapInt16HostToBig(*valuePtr)); + _cppThreadOperationalDataset.SetPanId(CFSwapInt16HostToBig(panID)); return YES; } @@ -124,7 +124,7 @@ - (nullable instancetype)initWithData:(NSData *)data // len+1 for null termination char networkName[MTRSizeThreadNetworkName + 1]; uint8_t pskc[MTRSizeThreadPSKc]; - uint8_t extendedPANID[MTRSizeThreadExtendedPanId]; + uint8_t extendedPANID[MTRSizeThreadExtendedPANID]; uint8_t masterKey[MTRSizeThreadMasterKey]; uint16_t panID; uint16_t channel; @@ -137,7 +137,7 @@ - (nullable instancetype)initWithData:(NSData *)data panID = CFSwapInt16BigToHost(panID); return [self initWithNetworkName:[NSString stringWithUTF8String:networkName] - extendedPANID:[NSData dataWithBytes:extendedPANID length:MTRSizeThreadExtendedPanId] + extendedPANID:[NSData dataWithBytes:extendedPANID length:MTRSizeThreadExtendedPANID] masterKey:[NSData dataWithBytes:masterKey length:MTRSizeThreadMasterKey] PSKc:[NSData dataWithBytes:pskc length:MTRSizeThreadPSKc] channelNumber:@(channel) diff --git a/src/darwin/Framework/CHIPTests/MTRThreadOperationalDatasetTests.mm b/src/darwin/Framework/CHIPTests/MTRThreadOperationalDatasetTests.mm index de15aa2405a547..59f34f8c9ad97f 100644 --- a/src/darwin/Framework/CHIPTests/MTRThreadOperationalDatasetTests.mm +++ b/src/darwin/Framework/CHIPTests/MTRThreadOperationalDatasetTests.mm @@ -38,7 +38,7 @@ - (void)testThreadOperationalDataset const uint16_t panID = 0x28f4; MTRThreadOperationalDataset * dataset = [[MTRThreadOperationalDataset alloc] initWithNetworkName:@"TestNetwork" - extendedPANID:[NSData dataWithBytes:&extendedPANID length:MTRSizeThreadExtendedPanId] + extendedPANID:[NSData dataWithBytes:&extendedPANID length:MTRSizeThreadExtendedPANID] masterKey:[NSData dataWithBytes:&masterKey length:MTRSizeThreadMasterKey] PSKc:[NSData dataWithBytes:&PKSc length:MTRSizeThreadPSKc] channelNumber:@(25)