From e2a2f10905c7c43849a2f4e1fc82714212031dfe Mon Sep 17 00:00:00 2001 From: Fesseha Date: Fri, 2 Jun 2023 12:44:04 +0200 Subject: [PATCH] address PR comments Co-authored-by: Boris Zbarsky --- .../TimeSyncDataProvider.cpp | 56 +++++++++++-------- .../TimeSyncDataProvider.h | 2 +- .../time-synchronization-server.cpp | 25 ++++----- .../time-synchronization-server.h | 6 +- 4 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.cpp b/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.cpp index 02e1379d111ccf..a7a6b0332f420e 100644 --- a/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.cpp +++ b/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.cpp @@ -24,7 +24,7 @@ namespace chip { constexpr size_t kTrustedTimeSourceMaxSerializedSize = TLV::EstimateStructOverhead(sizeof(chip::FabricIndex), sizeof(chip::NodeId), sizeof(chip::EndpointId)); constexpr size_t kTimeZoneMaxSerializedSize = - TLV::EstimateStructOverhead(sizeof(int32_t), sizeof(uint64_t), 64); // 64 for name field + TLV::EstimateStructOverhead(sizeof(int32_t), sizeof(uint64_t), static_cast(64)); // 64 for name field constexpr size_t kDSTOffsetMaxSerializedSize = TLV::EstimateStructOverhead(sizeof(int32_t), sizeof(uint64_t), sizeof(uint64_t)); // Multiply the serialized size by the maximum number of list size and add 2 bytes for the array start and end. @@ -54,7 +54,7 @@ CHIP_ERROR TimeSyncDataProvider::LoadTrustedTimeSource(TrustedTimeSource & timeS TLV::TLVReader reader; - reader.Init(bufferSpan.data(), bufferSpan.size()); + reader.Init(bufferSpan); ReturnErrorOnFailure(reader.Next(TLV::AnonymousTag())); ReturnErrorOnFailure(timeSource.Decode(reader)); @@ -72,9 +72,12 @@ CHIP_ERROR TimeSyncDataProvider::StoreDefaultNtp(const CharSpan & defaultNtp) static_cast(defaultNtp.size())); } -CHIP_ERROR TimeSyncDataProvider::LoadDefaultNtp(MutableByteSpan & defaultNtp) +CHIP_ERROR TimeSyncDataProvider::LoadDefaultNtp(CharSpan & defaultNtp) { - return Load(DefaultStorageKeyAllocator::TSDefaultNTP().KeyName(), defaultNtp); + MutableByteSpan dntpSpan(const_cast(Uint8::from_const_char(defaultNtp.data())), defaultNtp.size()); + ReturnErrorOnFailure(Load(DefaultStorageKeyAllocator::TSDefaultNTP().KeyName(), dntpSpan)); + defaultNtp.reduce_size(dntpSpan.size()); + return CHIP_NO_ERROR; } CHIP_ERROR TimeSyncDataProvider::ClearDefaultNtp() @@ -105,51 +108,56 @@ CHIP_ERROR TimeSyncDataProvider::LoadTimeZone(TimeZone & timeZoneList, uint8_t & { uint8_t buffer[kTimeZoneListMaxSerializedSize]; MutableByteSpan bufferSpan(buffer); - size = 0; - CHIP_ERROR err = CHIP_NO_ERROR; + CHIP_ERROR err; + size = 0; ReturnErrorOnFailure(Load(DefaultStorageKeyAllocator::TSTimeZone().KeyName(), bufferSpan)); TLV::TLVReader reader; TLV::TLVType outerType; - reader.Init(bufferSpan.data(), bufferSpan.size()); + reader.Init(bufferSpan); ReturnErrorOnFailure(reader.Next(TLV::TLVType::kTLVType_Array, TLV::AnonymousTag())); ReturnErrorOnFailure(reader.EnterContainer(outerType)); - auto tz = timeZoneList.begin(); uint8_t i = 0; - while (reader.Next() != CHIP_ERROR_END_OF_TLV && i < timeZoneList.size()) + while ((err = reader.Next()) == CHIP_NO_ERROR && i < timeZoneList.size()) { + auto & tz = timeZoneList[i]; app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type timeZone; ReturnErrorOnFailure(timeZone.Decode(reader)); - tz[i].offset = timeZone.offset; - tz[i].validAt = timeZone.validAt; + tz.offset = timeZone.offset; + tz.validAt = timeZone.validAt; if (timeZone.name.HasValue()) { - if (timeZone.name.Value().size() <= tz[i].name.Value().size()) + if (tz.name.HasValue() && timeZone.name.Value().size() <= tz.name.Value().size()) { - char * dest = const_cast(tz[i].name.Value().data()); + char * dest = const_cast(tz.name.Value().data()); size_t len = timeZone.name.Value().size(); memcpy(dest, timeZone.name.Value().data(), len); - tz[i].name.SetValue(chip::CharSpan(tz[i].name.Value().data(), len)); + tz.name.SetValue(chip::CharSpan(tz.name.Value().data(), len)); } else { - err = CHIP_ERROR_BUFFER_TOO_SMALL; + return CHIP_ERROR_BUFFER_TOO_SMALL; } } else { - tz[i].name.ClearValue(); + tz.name.ClearValue(); } i++; } + VerifyOrReturnError(err == CHIP_END_OF_TLV, CHIP_IM_GLOBAL_STATUS(ConstraintError)); ReturnErrorOnFailure(reader.ExitContainer(outerType)); + + err = reader.Next(); + VerifyOrReturnError(err == CHIP_END_OF_TLV, CHIP_IM_GLOBAL_STATUS(ConstraintError)); + size = i; - return err; + return CHIP_NO_ERROR; } CHIP_ERROR TimeSyncDataProvider::ClearTimeZone() @@ -182,6 +190,7 @@ CHIP_ERROR TimeSyncDataProvider::LoadDSTOffset(DSTOffset & dstOffsetList, uint8_ { uint8_t buffer[kDSTOffsetListMaxSerializedSize]; MutableByteSpan bufferSpan(buffer); + CHIP_ERROR err; size = 0; ReturnErrorOnFailure(Load(DefaultStorageKeyAllocator::TSDSTOffset().KeyName(), bufferSpan)); @@ -189,21 +198,24 @@ CHIP_ERROR TimeSyncDataProvider::LoadDSTOffset(DSTOffset & dstOffsetList, uint8_ TLV::TLVReader reader; TLV::TLVType outerType; - reader.Init(bufferSpan.data(), bufferSpan.size()); + reader.Init(bufferSpan); ReturnErrorOnFailure(reader.Next(TLV::TLVType::kTLVType_Array, TLV::AnonymousTag())); ReturnErrorOnFailure(reader.EnterContainer(outerType)); auto dst = dstOffsetList.begin(); uint8_t i = 0; - while (reader.Next() != CHIP_ERROR_END_OF_TLV && i < dstOffsetList.size()) + while ((err = reader.Next()) == CHIP_NO_ERROR && i < dstOffsetList.size()) { - app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type dstOffset; - ReturnErrorOnFailure(dstOffset.Decode(reader)); - dst[i] = dstOffset; + ReturnErrorOnFailure(dst[i].Decode(reader)); i++; } + VerifyOrReturnError(err == CHIP_END_OF_TLV, CHIP_IM_GLOBAL_STATUS(ConstraintError)); ReturnErrorOnFailure(reader.ExitContainer(outerType)); + + err = reader.Next(); + VerifyOrReturnError(err == CHIP_END_OF_TLV, CHIP_IM_GLOBAL_STATUS(ConstraintError)); + size = i; return CHIP_NO_ERROR; diff --git a/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.h b/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.h index 6e785eb7e7741f..2b09101785227f 100644 --- a/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.h +++ b/src/app/clusters/time-synchronization-server/TimeSyncDataProvider.h @@ -47,7 +47,7 @@ class TimeSyncDataProvider CHIP_ERROR ClearTrustedTimeSource(); CHIP_ERROR StoreDefaultNtp(const CharSpan & defaultNtp); - CHIP_ERROR LoadDefaultNtp(MutableByteSpan & defaultNtp); + CHIP_ERROR LoadDefaultNtp(CharSpan & defaultNtp); CHIP_ERROR ClearDefaultNtp(); CHIP_ERROR StoreTimeZone(const TimeZone & timeZoneList); diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index 5a152e4b36fd2c..4d315fbb0738dd 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -473,21 +473,19 @@ DataModel::Nullable return mTrustedTimeSource; } -CHIP_ERROR TimeSynchronizationServer::GetDefaultNtp(MutableByteSpan & dntp) +CHIP_ERROR TimeSynchronizationServer::GetDefaultNtp(CharSpan & dntp) { return mTimeSyncDataProvider.LoadDefaultNtp(dntp); } -DataModel::List & TimeSynchronizationServer::GetTimeZone() +DataModel::List TimeSynchronizationServer::GetTimeZone() { - mTimeZoneList = DataModel::List(mTz, mTimeZoneListSize); - return mTimeZoneList; + return mTimeZoneList.SubSpan(0, mTimeZoneListSize); } -DataModel::List & TimeSynchronizationServer::GetDSTOffset() +DataModel::List TimeSynchronizationServer::GetDSTOffset() { - mDstOffsetList = DataModel::List(mDst, mDstOffsetListSize); - return mDstOffsetList; + return mDstOffsetList.SubSpan(0, mDstOffsetListSize); } void TimeSynchronizationServer::ScheduleDelayedAction(System::Clock::Seconds32 delay, System::TimerCompleteCallback action, @@ -551,8 +549,7 @@ TimeState TimeSynchronizationServer::GetUpdatedTimeZoneState() mTimeZoneListSize = static_cast(tzList.size() - activeTzIndex); auto newTimeZoneList = tzList.SubSpan(activeTzIndex); VerifyOrReturnValue(mTimeSyncDataProvider.StoreTimeZone(newTimeZoneList) == CHIP_NO_ERROR, TimeState::kInvalid); - VerifyOrReturnValue(mTimeSyncDataProvider.LoadTimeZone(TimeSynchronizationServer::Instance().GetTimeZone(), - mTimeZoneListSize) == CHIP_NO_ERROR, + VerifyOrReturnValue(mTimeSyncDataProvider.LoadTimeZone(mTimeZoneList, mTimeZoneListSize) == CHIP_NO_ERROR, TimeState::kInvalid); return TimeState::kChanged; } @@ -587,8 +584,7 @@ TimeState TimeSynchronizationServer::GetUpdatedDSTOffsetState() mDstOffsetListSize = static_cast(dstList.size() - activeDstIndex); auto newDstOffsetList = dstList.SubSpan(activeDstIndex); VerifyOrReturnValue(mTimeSyncDataProvider.StoreDSTOffset(newDstOffsetList) == CHIP_NO_ERROR, TimeState::kInvalid); - VerifyOrReturnValue(mTimeSyncDataProvider.LoadDSTOffset(TimeSynchronizationServer::Instance().GetDSTOffset(), - mDstOffsetListSize) == CHIP_NO_ERROR, + VerifyOrReturnValue(mTimeSyncDataProvider.LoadDSTOffset(mDstOffsetList, mDstOffsetListSize) == CHIP_NO_ERROR, TimeState::kInvalid); return TimeState::kChanged; } @@ -635,12 +631,11 @@ CHIP_ERROR TimeSynchronizationAttrAccess::ReadTrustedTimeSource(EndpointId endpo CHIP_ERROR TimeSynchronizationAttrAccess::ReadDefaultNtp(EndpointId endpoint, AttributeValueEncoder & aEncoder) { CHIP_ERROR err = CHIP_NO_ERROR; - uint8_t buffer[DefaultNTP::TypeInfo::MaxLength()]; - MutableByteSpan dntp(buffer); + char buffer[DefaultNTP::TypeInfo::MaxLength()]; + chip::CharSpan dntp(buffer); if (TimeSynchronizationServer::Instance().GetDefaultNtp(dntp) == CHIP_NO_ERROR && dntp.size() != 0) { - const char * charBuf = reinterpret_cast(buffer); - err = aEncoder.Encode(chip::CharSpan(charBuf, dntp.size())); + err = aEncoder.Encode(dntp); } else { diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.h b/src/app/clusters/time-synchronization-server/time-synchronization-server.h index 065fc64c0919c9..7fde02700c51f8 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.h @@ -94,9 +94,9 @@ class TimeSynchronizationServer CHIP_ERROR SetDSTOffset(DataModel::DecodableList dstL); CHIP_ERROR ClearDSTOffset(void); DataModel::Nullable & GetTrustedTimeSource(void); - CHIP_ERROR GetDefaultNtp(MutableByteSpan & dntp); - DataModel::List & GetTimeZone(void); - DataModel::List & GetDSTOffset(void); + CHIP_ERROR GetDefaultNtp(CharSpan & dntp); + DataModel::List GetTimeZone(void); + DataModel::List GetDSTOffset(void); CHIP_ERROR SetUTCTime(chip::EndpointId ep, uint64_t utcTime, TimeSynchronization::GranularityEnum granularity, TimeSynchronization::TimeSourceEnum source);