From 20831134a45b76cc78bc4ecf3f1e5fd57735e97f Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 1 Jul 2022 17:32:47 -0400 Subject: [PATCH] Clean up optional arguments properly in chip-tool. (#20190) Otherwise in interactive mode we can end up with noise left over from a previous command invocation. Fixes https://github.com/project-chip/connectedhomeip/issues/20151 --- .../commands/clusters/ModelCommand.cpp | 3 +- .../chip-tool/commands/common/CHIPCommand.h | 2 +- .../chip-tool/commands/common/Command.cpp | 160 +++++++++++++++--- .../discover/DiscoverCommissionersCommand.cpp | 2 + 4 files changed, 140 insertions(+), 27 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ModelCommand.cpp b/examples/chip-tool/commands/clusters/ModelCommand.cpp index 9c06ed70b0edd1..fd255abfe204b0 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.cpp +++ b/examples/chip-tool/commands/clusters/ModelCommand.cpp @@ -66,7 +66,8 @@ void ModelCommand::OnDeviceConnectionFailureFn(void * context, PeerId peerId, CH void ModelCommand::Shutdown() { - ResetArguments(); mOnDeviceConnectedCallback.Cancel(); mOnDeviceConnectionFailureCallback.Cancel(); + + CHIPCommand::Shutdown(); } diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index e4cebf6095cc35..7688a34873bfbe 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -101,7 +101,7 @@ class CHIPCommand : public Command // Shut down the command. After a Shutdown call the command object is ready // to be used for another command invocation. - virtual void Shutdown() {} + virtual void Shutdown() { ResetArguments(); } // Clean up any resources allocated by the command. Some commands may hold // on to resources after Shutdown(), but Cleanup() will guarantee those are diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp index 1931d88e39752f..c8ee1850a729e1 100644 --- a/examples/chip-tool/commands/common/Command.cpp +++ b/examples/chip-tool/commands/common/Command.cpp @@ -854,44 +854,154 @@ size_t Command::AddArgumentToList(Argument && argument) return 0; } +namespace { +template +void ResetOptionalArg(const Argument & arg) +{ + VerifyOrDie(arg.isOptional()); + + if (arg.isNullable()) + { + reinterpret_cast> *>(arg.value)->ClearValue(); + } + else + { + reinterpret_cast *>(arg.value)->ClearValue(); + } +} +} // anonymous namespace + void Command::ResetArguments() { for (size_t i = 0; i < mArgs.size(); i++) { const Argument arg = mArgs[i]; const ArgumentType type = arg.type; - const uint8_t flags = arg.flags; - if (type == ArgumentType::VectorBool && flags == Argument::kOptional) + if (arg.isOptional()) { - auto vectorArgument = static_cast *>(arg.value); - vectorArgument->clear(); - } - else if (type == ArgumentType::Vector16 && flags != Argument::kOptional) - { - auto vectorArgument = static_cast *>(arg.value); - vectorArgument->clear(); - } - else if (type == ArgumentType::Vector32 && flags != Argument::kOptional) - { - auto vectorArgument = static_cast *>(arg.value); - vectorArgument->clear(); - } - else if (type == ArgumentType::Vector32 && flags == Argument::kOptional) - { - auto optionalArgument = static_cast> *>(arg.value); - if (optionalArgument->HasValue()) + // Must always clean these up so they don't carry over to the next + // command invocation in interactive mode. + switch (type) { - optionalArgument->Value().clear(); + case ArgumentType::Complex: { + // No optional complex arguments so far. + VerifyOrDie(false); + break; + } + case ArgumentType::Custom: { + // No optional custom arguments so far. + VerifyOrDie(false); + break; + } + case ArgumentType::VectorBool: { + auto vectorArgument = static_cast *>(arg.value); + vectorArgument->clear(); + break; + } + case ArgumentType::Vector16: { + // No optional Vector16 arguments so far. + VerifyOrDie(false); + break; + } + case ArgumentType::Vector32: { + ResetOptionalArg>(arg); + break; + } + case ArgumentType::VectorCustom: { + // No optional VectorCustom arguments so far. + VerifyOrDie(false); + break; + } + case ArgumentType::Attribute: { + // No optional Attribute arguments so far. + VerifyOrDie(false); + break; + } + case ArgumentType::String: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::CharString: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::OctetString: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Bool: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_uint8: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_uint16: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_uint32: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_uint64: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_int8: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_int16: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_int32: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Number_int64: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Float: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Double: { + ResetOptionalArg(arg); + break; + } + case ArgumentType::Address: { + ResetOptionalArg(arg); + break; + } } } - else if (type == ArgumentType::VectorCustom && flags != Argument::kOptional) + else { - auto vectorArgument = static_cast *>(arg.value); - for (auto & customArgument : *vectorArgument) + // Some non-optional arguments have state that needs to be cleaned + // up too. + if (type == ArgumentType::Vector16) { - delete customArgument; + auto vectorArgument = static_cast *>(arg.value); + vectorArgument->clear(); + } + else if (type == ArgumentType::Vector32) + { + auto vectorArgument = static_cast *>(arg.value); + vectorArgument->clear(); + } + else if (type == ArgumentType::VectorCustom) + { + auto vectorArgument = static_cast *>(arg.value); + for (auto & customArgument : *vectorArgument) + { + delete customArgument; + } + vectorArgument->clear(); } - vectorArgument->clear(); } } } diff --git a/examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp b/examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp index 3cb3cf6153f129..b2fe353931328a 100644 --- a/examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp +++ b/examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp @@ -42,4 +42,6 @@ void DiscoverCommissionersCommand::Shutdown() ChipLogProgress(chipTool, "Total of %d commissioner(s) discovered in %u sec", commissionerCount, std::chrono::duration_cast(GetWaitDuration()).count()); + + CHIPCommand::Shutdown(); }