From 093bc7f0375ca25281d4820ed81db80b00ff23cb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Jul 2023 09:24:45 -0400 Subject: [PATCH] Remove raw value access from TLV::Tag (#27869) * Provide a AppendTo for formatting TLV tags * Restyle * Fix uni tests to new formatting * Restyle * Fix tag formatting when enter container fails --- src/lib/core/BUILD.gn | 1 + src/lib/core/TLVTags.cpp | 48 +++++++++++++ src/lib/core/TLVTags.h | 4 +- src/lib/format/protocol_decoder.cpp | 61 ++-------------- src/lib/format/tests/TestDecoding.cpp | 100 +++++++++++++------------- 5 files changed, 109 insertions(+), 105 deletions(-) create mode 100644 src/lib/core/TLVTags.cpp diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index edff299efd8ea6..5a08f7d1935a23 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -109,6 +109,7 @@ static_library("core") { "TLVCircularBuffer.h", "TLVDebug.cpp", "TLVReader.cpp", + "TLVTags.cpp", "TLVTags.h", "TLVTypes.h", "TLVUpdater.cpp", diff --git a/src/lib/core/TLVTags.cpp b/src/lib/core/TLVTags.cpp new file mode 100644 index 00000000000000..8d3e744b4667a3 --- /dev/null +++ b/src/lib/core/TLVTags.cpp @@ -0,0 +1,48 @@ +/* + * + * Copyright (c) 2020-2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "TLVTags.h" + +namespace chip { +namespace TLV { + +/// Appends the text representation for a tag to the given string builder base. +StringBuilderBase & Tag::AppendTo(StringBuilderBase & out) +{ + if (TLV::IsProfileTag(*this)) + { + out.AddFormat("ProfileTag(0x%X::0x%X::0x%" PRIX32 ")", TLV::VendorIdFromTag(*this), TLV::ProfileNumFromTag(*this), + TLV::TagNumFromTag(*this)); + } + else if (TLV::IsContextTag(*this)) + { + out.AddFormat("ContextTag(0x%" PRIX32 ")", TLV::TagNumFromTag(*this)); + } + else if (*this == TLV::AnonymousTag()) + { + out.Add("AnonymousTag()"); + } + else + { + out.AddFormat("UnknownTag(0x%" PRIX64 ")", mVal); + } + + return out; +} + +} // namespace TLV +} // namespace chip diff --git a/src/lib/core/TLVTags.h b/src/lib/core/TLVTags.h index fd2a8152b411f2..b82be9179466af 100644 --- a/src/lib/core/TLVTags.h +++ b/src/lib/core/TLVTags.h @@ -25,6 +25,7 @@ #pragma once #include +#include #include #include @@ -46,7 +47,8 @@ class Tag constexpr bool operator==(const Tag & other) const { return mVal == other.mVal; } constexpr bool operator!=(const Tag & other) const { return mVal != other.mVal; } - uint64_t RawValue() const { return mVal; } + /// Appends the text representation of the tag to the given string builder base. + StringBuilderBase & AppendTo(StringBuilderBase & out); private: explicit constexpr Tag(uint64_t val) : mVal(val) {} diff --git a/src/lib/format/protocol_decoder.cpp b/src/lib/format/protocol_decoder.cpp index 8c3b352133902a..97d6dd857d9198 100644 --- a/src/lib/format/protocol_decoder.cpp +++ b/src/lib/format/protocol_decoder.cpp @@ -45,51 +45,6 @@ class ByTag const Tag mTag; }; -const char * DecodeTagControl(const TLVTagControl aTagControl) -{ - switch (aTagControl) - { - case TLVTagControl::Anonymous: - return "Anonymous"; - case TLVTagControl::ContextSpecific: - return "ContextSpecific"; - case TLVTagControl::CommonProfile_2Bytes: - return "Common2B"; - case TLVTagControl::CommonProfile_4Bytes: - return "Common4B"; - case TLVTagControl::ImplicitProfile_2Bytes: - return "Implicit2B"; - case TLVTagControl::ImplicitProfile_4Bytes: - return "Implicit4B"; - case TLVTagControl::FullyQualified_6Bytes: - return "FullyQualified6B"; - case TLVTagControl::FullyQualified_8Bytes: - return "FullyQualified8"; - default: - return "???"; - } -} - -void FormatCurrentTag(const TLVReader & reader, chip::StringBuilderBase & out) -{ - chip::TLV::TLVTagControl tagControl = static_cast(reader.GetControlByte() & kTLVTagControlMask); - chip::TLV::Tag tag = reader.GetTag(); - - if (IsProfileTag(tag)) - { - out.AddFormat("%s(0x%X::0x%X::0x%" PRIX32 ")", DecodeTagControl(tagControl), VendorIdFromTag(tag), ProfileNumFromTag(tag), - TagNumFromTag(tag)); - } - else if (IsContextTag(tag)) - { - out.AddFormat("%s(0x%" PRIX32 ")", DecodeTagControl(tagControl), TagNumFromTag(tag)); - } - else - { - out.AddFormat("UnknownTag(0x%" PRIX64 ")", tag.RawValue()); - } -} - CHIP_ERROR FormatCurrentValue(TLVReader & reader, chip::StringBuilderBase & out) { switch (reader.GetType()) @@ -351,6 +306,7 @@ bool PayloadDecoderBase::ReaderEnterContainer(PayloadEntry & entry) if (mCurrentNesting >= kMaxDecodeDepth) { mValueBuilder.AddFormat("NESTING DEPTH REACHED"); + mReader.GetTag().AppendTo(mNameBuilder.Reset()); entry = PayloadEntry::SimpleValue(mNameBuilder.c_str(), mValueBuilder.c_str()); return false; } @@ -360,6 +316,7 @@ bool PayloadDecoderBase::ReaderEnterContainer(PayloadEntry & entry) if (err != CHIP_NO_ERROR) { mValueBuilder.AddFormat("ERROR entering container: %" CHIP_ERROR_FORMAT, err.Format()); + mReader.GetTag().AppendTo(mNameBuilder.Reset()); // assume enter is not done, so tag is correct entry = PayloadEntry::SimpleValue(mNameBuilder.c_str(), mValueBuilder.c_str()); mState = State::kDone; return false; @@ -372,13 +329,8 @@ bool PayloadDecoderBase::ReaderEnterContainer(PayloadEntry & entry) void PayloadDecoderBase::EnterContainer(PayloadEntry & entry) { - // must be done BEFORE entering container - // to preserve the value and not get a 'container tag' - // below when data is not valid - // - // TODO: this formatting is wasteful, should really be done only - // if data is NULLPTR. - FormatCurrentTag(mReader, mNameBuilder.Reset()); + // Tag fetch must be done BEFORE entering container + chip::TLV::Tag tag = mReader.GetTag(); VerifyOrReturn(ReaderEnterContainer(entry)); @@ -396,6 +348,7 @@ void PayloadDecoderBase::EnterContainer(PayloadEntry & entry) if (data == nullptr) { + tag.AppendTo(mNameBuilder.Reset()); entry = PayloadEntry::NestingEnter(mNameBuilder.c_str()); } else @@ -454,7 +407,7 @@ void PayloadDecoderBase::NextFromContentRead(PayloadEntry & entry) if (data == nullptr) { - FormatCurrentTag(mReader, mNameBuilder.Reset()); + mReader.GetTag().AppendTo(mNameBuilder.Reset()); entry = PayloadEntry::SimpleValue(mNameBuilder.c_str(), mValueBuilder.c_str()); return; } @@ -600,7 +553,7 @@ void PayloadDecoderBase::NextFromValueRead(PayloadEntry & entry) if (data == nullptr) { - FormatCurrentTag(mReader, mNameBuilder.Reset()); + mReader.GetTag().AppendTo(mNameBuilder.Reset()); PrettyPrintCurrentValue(mReader, mValueBuilder.Reset(), mPayloadPosition); entry = PayloadEntry::SimpleValue(mNameBuilder.c_str(), mValueBuilder.c_str()); mPayloadPosition.Exit(); diff --git a/src/lib/format/tests/TestDecoding.cpp b/src/lib/format/tests/TestDecoding.cpp index 6fcb154de7430d..f9ef31719f5b6b 100644 --- a/src/lib/format/tests/TestDecoding.cpp +++ b/src/lib/format/tests/TestDecoding.cpp @@ -505,14 +505,14 @@ void TestEmptyClusterMetaDataDecode(nlTestSuite * inSuite, void * inContext) " endpoint_id: 0\n" " cluster_id: 31\n" " attribute_id: 0\n" - " 0x1f::ATTR(0x0)\n" // Cluster 31, attribute 0 - " UnknownTag(0x100)\n" // List entry (acl is a list) - " ContextSpecific(0x1): 5\n" // privilege - " ContextSpecific(0x2): 2\n" // authMode - " ContextSpecific(0x3)\n" // subjects - " UnknownTag(0x100): 112233\n" // List entry (subjects is a list) - " ContextSpecific(0x4): NULL\n" // targets - " ContextSpecific(0xFE): 1\n" // fabricIndex + " 0x1f::ATTR(0x0)\n" // Cluster 31, attribute 0 + " AnonymousTag()\n" // List entry (acl is a list) + " ContextTag(0x1): 5\n" // privilege + " ContextTag(0x2): 2\n" // authMode + " ContextTag(0x3)\n" // subjects + " AnonymousTag(): 112233\n" // List entry (subjects is a list) + " ContextTag(0x4): NULL\n" // targets + " ContextTag(0xFE): 1\n" // fabricIndex " suppress_response: true\n" " interaction_model_revison: 1\n"); } @@ -536,24 +536,24 @@ void TestWrongDecodeData(nlTestSuite * inSuite, void * inContext) TestSampleData(inSuite, params, secure_channel_mrp_ack, "proto16: EMPTY\n"); TestSampleData(inSuite, params, im_protocol_report_data_acl, "proto5\n" - " ContextSpecific(0x1)\n" - " UnknownTag(0x100)\n" - " ContextSpecific(0x1)\n" - " ContextSpecific(0x0): 3420147058\n" - " ContextSpecific(0x1)\n" - " ContextSpecific(0x2): 0\n" - " ContextSpecific(0x3): 31\n" - " ContextSpecific(0x4): 0\n" - " ContextSpecific(0x2)\n" - " UnknownTag(0x100)\n" - " ContextSpecific(0x1): 5\n" - " ContextSpecific(0x2): 2\n" - " ContextSpecific(0x3)\n" - " UnknownTag(0x100): 112233\n" - " ContextSpecific(0x4): NULL\n" - " ContextSpecific(0xFE): 1\n" - " ContextSpecific(0x4): true\n" - " ContextSpecific(0xFF): 1\n"); + " ContextTag(0x1)\n" + " AnonymousTag()\n" + " ContextTag(0x1)\n" + " ContextTag(0x0): 3420147058\n" + " ContextTag(0x1)\n" + " ContextTag(0x2): 0\n" + " ContextTag(0x3): 31\n" + " ContextTag(0x4): 0\n" + " ContextTag(0x2)\n" + " AnonymousTag()\n" + " ContextTag(0x1): 5\n" + " ContextTag(0x2): 2\n" + " ContextTag(0x3)\n" + " AnonymousTag(): 112233\n" + " ContextTag(0x4): NULL\n" + " ContextTag(0xFE): 1\n" + " ContextTag(0x4): true\n" + " ContextTag(0xFF): 1\n"); } void TestNestingOverflow(nlTestSuite * inSuite, void * inContext) @@ -629,30 +629,30 @@ void TestNestingOverflow(nlTestSuite * inSuite, void * inContext) TestSampleData(inSuite, params, fake_payload, "proto5\n" - " ContextSpecific(0x0)\n" - " ContextSpecific(0x1)\n" - " ContextSpecific(0x2)\n" - " ContextSpecific(0x3)\n" - " ContextSpecific(0x4)\n" - " ContextSpecific(0x5)\n" - " ContextSpecific(0x6)\n" - " ContextSpecific(0x7)\n" - " ContextSpecific(0x8)\n" - " ContextSpecific(0x9)\n" - " ContextSpecific(0xA)\n" - " ContextSpecific(0xB)\n" - " ContextSpecific(0xC)\n" - " ContextSpecific(0xD)\n" - " ContextSpecific(0xE)\n" - " ContextSpecific(0xF): NESTING DEPTH REACHED\n" - " ContextSpecific(0x20)\n" - " ContextSpecific(0x21)\n" - " ContextSpecific(0x22)\n" - " ContextSpecific(0x23)\n" - " ContextSpecific(0x30)\n" - " ContextSpecific(0x31)\n" - " ContextSpecific(0x32)\n" - " ContextSpecific(0x33)\n"); + " ContextTag(0x0)\n" + " ContextTag(0x1)\n" + " ContextTag(0x2)\n" + " ContextTag(0x3)\n" + " ContextTag(0x4)\n" + " ContextTag(0x5)\n" + " ContextTag(0x6)\n" + " ContextTag(0x7)\n" + " ContextTag(0x8)\n" + " ContextTag(0x9)\n" + " ContextTag(0xA)\n" + " ContextTag(0xB)\n" + " ContextTag(0xC)\n" + " ContextTag(0xD)\n" + " ContextTag(0xE)\n" + " ContextTag(0xF): NESTING DEPTH REACHED\n" + " ContextTag(0x20)\n" + " ContextTag(0x21)\n" + " ContextTag(0x22)\n" + " ContextTag(0x23)\n" + " ContextTag(0x30)\n" + " ContextTag(0x31)\n" + " ContextTag(0x32)\n" + " ContextTag(0x33)\n"); } const nlTest sTests[] = {