From 5638457032a811e8fe17ea2819a72d9d51bc0ec8 Mon Sep 17 00:00:00 2001 From: Vivien Nicolas Date: Tue, 7 Mar 2023 13:48:49 +0100 Subject: [PATCH] [chip-tool] Some CHIP_ERROR returned values are ignored (#25486) --- .../commands/clusters/CustomArgument.h | 2 +- .../commands/clusters/DataModelLogger.h | 23 +++------ .../chip-tool/commands/common/CHIPCommand.cpp | 11 +++- .../commands/common/RemoteDataModelLogger.cpp | 51 ++++++++++--------- .../DiscoverCommissionablesCommand.cpp | 4 +- .../interactive/InteractiveCommands.cpp | 5 +- .../commands/pairing/PairingCommand.cpp | 34 +++++++++---- .../payload/SetupPayloadGenerateCommand.cpp | 4 +- .../websocket-server/WebSocketServer.cpp | 3 +- .../common/websocket-server/WebSocketServer.h | 2 +- .../placeholder/linux/InteractiveServer.cpp | 8 +-- 11 files changed, 83 insertions(+), 64 deletions(-) diff --git a/examples/chip-tool/commands/clusters/CustomArgument.h b/examples/chip-tool/commands/clusters/CustomArgument.h index ab6ce40752ae76..81d1bbf7faa7bf 100644 --- a/examples/chip-tool/commands/clusters/CustomArgument.h +++ b/examples/chip-tool/commands/clusters/CustomArgument.h @@ -279,7 +279,7 @@ class CustomArgument { chip::TLV::TLVReader reader; reader.Init(mData, mDataLen); - reader.Next(); + ReturnErrorOnFailure(reader.Next()); return writer.CopyElement(tag, reader); } diff --git a/examples/chip-tool/commands/clusters/DataModelLogger.h b/examples/chip-tool/commands/clusters/DataModelLogger.h index 5ffb775d0bf01a..1d9d1e1ccd6955 100644 --- a/examples/chip-tool/commands/clusters/DataModelLogger.h +++ b/examples/chip-tool/commands/clusters/DataModelLogger.h @@ -103,26 +103,20 @@ class DataModelLogger template ::value, int> = 0> static CHIP_ERROR LogValue(const char * label, size_t indent, X value) { - DataModelLogger::LogValue(label, indent, chip::to_underlying(value)); - return CHIP_NO_ERROR; + return DataModelLogger::LogValue(label, indent, chip::to_underlying(value)); } template static CHIP_ERROR LogValue(const char * label, size_t indent, chip::BitFlags value) { - DataModelLogger::LogValue(label, indent, value.Raw()); - return CHIP_NO_ERROR; + return DataModelLogger::LogValue(label, indent, value.Raw()); } template static CHIP_ERROR LogValue(const char * label, size_t indent, const chip::app::DataModel::DecodableList & value) { - size_t count = 0; - CHIP_ERROR err = value.ComputeSize(&count); - if (err != CHIP_NO_ERROR) - { - return err; - } + size_t count = 0; + ReturnErrorOnFailure(value.ComputeSize(&count)); DataModelLogger::LogString(label, indent, std::to_string(count) + " entries"); auto iter = value.begin(); @@ -146,13 +140,10 @@ class DataModelLogger if (value.IsNull()) { DataModelLogger::LogString(label, indent, "null"); - } - else - { - DataModelLogger::LogValue(label, indent, value.Value()); + return CHIP_NO_ERROR; } - return CHIP_NO_ERROR; + return DataModelLogger::LogValue(label, indent, value.Value()); } template @@ -160,7 +151,7 @@ class DataModelLogger { if (value.HasValue()) { - DataModelLogger::LogValue(label, indent, value.Value()); + return DataModelLogger::LogValue(label, indent, value.Value()); } return CHIP_NO_ERROR; diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index c97f2c17090b64..498c61f6123912 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -506,7 +506,16 @@ CHIP_ERROR CHIPCommand::StartWaiting(chip::System::Clock::Timeout duration) mWaitingForResponse = true; } - chip::DeviceLayer::PlatformMgr().ScheduleWork(RunQueuedCommand, reinterpret_cast(this)); + auto err = chip::DeviceLayer::PlatformMgr().ScheduleWork(RunQueuedCommand, reinterpret_cast(this)); + if (CHIP_NO_ERROR != err) + { + { + std::lock_guard lk(cvWaitingForResponseMutex); + mWaitingForResponse = false; + } + return err; + } + auto waitingUntil = std::chrono::system_clock::now() + std::chrono::duration_cast(duration); { std::unique_lock lk(cvWaitingForResponseMutex); diff --git a/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp b/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp index d572dfc1ac2eeb..821370d43b5c3c 100644 --- a/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp +++ b/examples/chip-tool/commands/common/RemoteDataModelLogger.cpp @@ -169,48 +169,51 @@ CHIP_ERROR LogDiscoveredNodeData(const chip::Dnssd::DiscoveredNodeData & nodeDat { VerifyOrReturnError(gDelegate != nullptr, CHIP_NO_ERROR); - if (!chip::CanCastTo(nodeData.resolutionData.numIPs)) + auto & resolutionData = nodeData.resolutionData; + auto & commissionData = nodeData.commissionData; + + if (!chip::CanCastTo(resolutionData.numIPs)) { ChipLogError(chipTool, "Too many ips."); return CHIP_ERROR_INVALID_ARGUMENT; } - if (!chip::CanCastTo(nodeData.commissionData.rotatingIdLen)) + if (!chip::CanCastTo(commissionData.rotatingIdLen)) { ChipLogError(chipTool, "Can not convert rotatingId to json format."); return CHIP_ERROR_INVALID_ARGUMENT; } char rotatingId[chip::Dnssd::kMaxRotatingIdLen * 2 + 1] = ""; - chip::Encoding::BytesToUppercaseHexString(nodeData.commissionData.rotatingId, nodeData.commissionData.rotatingIdLen, rotatingId, - sizeof(rotatingId)); + ReturnErrorOnFailure(chip::Encoding::BytesToUppercaseHexString(commissionData.rotatingId, commissionData.rotatingIdLen, + rotatingId, sizeof(rotatingId))); Json::Value value; - value["hostName"] = nodeData.resolutionData.hostName; - value["instanceName"] = nodeData.commissionData.instanceName; - value["longDiscriminator"] = nodeData.commissionData.longDiscriminator; - value["shortDiscriminator"] = ((nodeData.commissionData.longDiscriminator >> 8) & 0x0F); - value["vendorId"] = nodeData.commissionData.vendorId; - value["productId"] = nodeData.commissionData.productId; - value["commissioningMode"] = nodeData.commissionData.commissioningMode; - value["deviceType"] = nodeData.commissionData.deviceType; - value["deviceName"] = nodeData.commissionData.deviceName; + value["hostName"] = resolutionData.hostName; + value["instanceName"] = commissionData.instanceName; + value["longDiscriminator"] = commissionData.longDiscriminator; + value["shortDiscriminator"] = ((commissionData.longDiscriminator >> 8) & 0x0F); + value["vendorId"] = commissionData.vendorId; + value["productId"] = commissionData.productId; + value["commissioningMode"] = commissionData.commissioningMode; + value["deviceType"] = commissionData.deviceType; + value["deviceName"] = commissionData.deviceName; value["rotatingId"] = rotatingId; - value["rotatingIdLen"] = static_cast(nodeData.commissionData.rotatingIdLen); - value["pairingHint"] = nodeData.commissionData.pairingHint; - value["pairingInstruction"] = nodeData.commissionData.pairingInstruction; - value["supportsTcp"] = nodeData.resolutionData.supportsTcp; - value["port"] = nodeData.resolutionData.port; - value["numIPs"] = static_cast(nodeData.resolutionData.numIPs); - - if (nodeData.resolutionData.mrpRetryIntervalIdle.HasValue()) + value["rotatingIdLen"] = static_cast(commissionData.rotatingIdLen); + value["pairingHint"] = commissionData.pairingHint; + value["pairingInstruction"] = commissionData.pairingInstruction; + value["supportsTcp"] = resolutionData.supportsTcp; + value["port"] = resolutionData.port; + value["numIPs"] = static_cast(resolutionData.numIPs); + + if (resolutionData.mrpRetryIntervalIdle.HasValue()) { - value["mrpRetryIntervalIdle"] = nodeData.resolutionData.mrpRetryIntervalIdle.Value().count(); + value["mrpRetryIntervalIdle"] = resolutionData.mrpRetryIntervalIdle.Value().count(); } - if (nodeData.resolutionData.mrpRetryIntervalActive.HasValue()) + if (resolutionData.mrpRetryIntervalActive.HasValue()) { - value["mrpRetryIntervalActive"] = nodeData.resolutionData.mrpRetryIntervalActive.Value().count(); + value["mrpRetryIntervalActive"] = resolutionData.mrpRetryIntervalActive.Value().count(); } Json::Value rootValue; diff --git a/examples/chip-tool/commands/discover/DiscoverCommissionablesCommand.cpp b/examples/chip-tool/commands/discover/DiscoverCommissionablesCommand.cpp index b8b7ca6feba757..035abae0769828 100644 --- a/examples/chip-tool/commands/discover/DiscoverCommissionablesCommand.cpp +++ b/examples/chip-tool/commands/discover/DiscoverCommissionablesCommand.cpp @@ -30,8 +30,8 @@ void DiscoverCommissionablesCommandBase::OnDiscoveredDevice(const chip::Dnssd::D if (mDiscoverOnce.ValueOr(true)) { mCommissioner->RegisterDeviceDiscoveryDelegate(nullptr); - mCommissioner->StopCommissionableDiscovery(); - SetCommandExitStatus(CHIP_NO_ERROR); + auto err = mCommissioner->StopCommissionableDiscovery(); + SetCommandExitStatus(err); } } diff --git a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp index 599de21af3e8b5..0a1f1957a6df52 100644 --- a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp +++ b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp @@ -306,7 +306,10 @@ bool InteractiveCommand::ParseCommand(char * command, int * status) { if (strcmp(command, kInteractiveModeStopCommand) == 0) { - chip::DeviceLayer::PlatformMgr().ScheduleWork(ExecuteDeferredCleanups, 0); + // If scheduling the cleanup fails, there is not much we can do. + // But if something went wrong while the application is leaving it could be because things have + // not been cleaned up properly, so it is still useful to log the failure. + LogErrorOnFailure(chip::DeviceLayer::PlatformMgr().ScheduleWork(ExecuteDeferredCleanups, 0)); return false; } diff --git a/examples/chip-tool/commands/pairing/PairingCommand.cpp b/examples/chip-tool/commands/pairing/PairingCommand.cpp index b331c1e013c225..5dabeb69837381 100644 --- a/examples/chip-tool/commands/pairing/PairingCommand.cpp +++ b/examples/chip-tool/commands/pairing/PairingCommand.cpp @@ -269,22 +269,32 @@ void PairingCommand::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err) void PairingCommand::OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData & nodeData) { - // Ignore nodes with closed comissioning window + // Ignore nodes with closed commissioning window VerifyOrReturn(nodeData.commissionData.commissioningMode != 0); - const uint16_t port = nodeData.resolutionData.port; + auto & resolutionData = nodeData.resolutionData; + + const uint16_t port = resolutionData.port; char buf[chip::Inet::IPAddress::kMaxStringLength]; - nodeData.resolutionData.ipAddress[0].ToString(buf); + resolutionData.ipAddress[0].ToString(buf); ChipLogProgress(chipTool, "Discovered Device: %s:%u", buf, port); // Stop Mdns discovery. - CurrentCommissioner().StopCommissionableDiscovery(); + auto err = CurrentCommissioner().StopCommissionableDiscovery(); + + // Some platforms does not implement a mechanism to stop mdns browse, so + // we just ignore CHIP_ERROR_NOT_IMPLEMENTED instead of bailing out. + if (CHIP_NO_ERROR != err && CHIP_ERROR_NOT_IMPLEMENTED != err) + { + SetCommandExitStatus(err); + return; + } + CurrentCommissioner().RegisterDeviceDiscoveryDelegate(nullptr); - Inet::InterfaceId interfaceId = - nodeData.resolutionData.ipAddress[0].IsIPv6LinkLocal() ? nodeData.resolutionData.interfaceId : Inet::InterfaceId::Null(); - PeerAddress peerAddress = PeerAddress::UDP(nodeData.resolutionData.ipAddress[0], port, interfaceId); - CHIP_ERROR err = Pair(mNodeId, peerAddress); + auto interfaceId = resolutionData.ipAddress[0].IsIPv6LinkLocal() ? resolutionData.interfaceId : Inet::InterfaceId::Null(); + auto peerAddress = PeerAddress::UDP(resolutionData.ipAddress[0], port, interfaceId); + err = Pair(mNodeId, peerAddress); if (CHIP_NO_ERROR != err) { SetCommandExitStatus(err); @@ -320,6 +330,10 @@ void PairingCommand::OnDeviceAttestationCompleted(chip::Controller::DeviceCommis chip::Credentials::AttestationVerificationResult attestationResult) { // Bypass attestation verification, continue with success - deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, - chip::Credentials::AttestationVerificationResult::kSuccess); + auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation( + device, chip::Credentials::AttestationVerificationResult::kSuccess); + if (CHIP_NO_ERROR != err) + { + SetCommandExitStatus(err); + } } diff --git a/examples/chip-tool/commands/payload/SetupPayloadGenerateCommand.cpp b/examples/chip-tool/commands/payload/SetupPayloadGenerateCommand.cpp index 093a83f4620bbd..c6afc4a4715be5 100644 --- a/examples/chip-tool/commands/payload/SetupPayloadGenerateCommand.cpp +++ b/examples/chip-tool/commands/payload/SetupPayloadGenerateCommand.cpp @@ -118,12 +118,12 @@ CHIP_ERROR SetupPayloadGenerateQRCodeCommand::PopulatePayloadTLVFromBytes(SetupP { // First clear out all the existing TVL bits from the payload. Ignore // errors here, because we don't care if those bits are not present. - payload.removeSerialNumber(); + (void) payload.removeSerialNumber(); auto existingVendorData = payload.getAllOptionalVendorData(); for (auto & data : existingVendorData) { - payload.removeOptionalVendorData(data.tag); + (void) payload.removeOptionalVendorData(data.tag); } if (tlvBytes.empty()) diff --git a/examples/common/websocket-server/WebSocketServer.cpp b/examples/common/websocket-server/WebSocketServer.cpp index 49c90fab5d1ad9..f5bbd973d407c9 100644 --- a/examples/common/websocket-server/WebSocketServer.cpp +++ b/examples/common/websocket-server/WebSocketServer.cpp @@ -200,9 +200,8 @@ bool WebSocketServer::OnWebSocketMessageReceived(char * msg) return shouldContinue; } -CHIP_ERROR WebSocketServer::Send(const char * msg) +void WebSocketServer::Send(const char * msg) { std::lock_guard lock(gMutex); gMessageQueue.push_back(msg); - return CHIP_NO_ERROR; } diff --git a/examples/common/websocket-server/WebSocketServer.h b/examples/common/websocket-server/WebSocketServer.h index 994f10fcbc1b79..b6a181ce0c2f30 100644 --- a/examples/common/websocket-server/WebSocketServer.h +++ b/examples/common/websocket-server/WebSocketServer.h @@ -29,7 +29,7 @@ class WebSocketServer : public WebSocketServerDelegate { public: CHIP_ERROR Run(chip::Optional port, WebSocketServerDelegate * delegate); - CHIP_ERROR Send(const char * msg); + void Send(const char * msg); bool OnWebSocketMessageReceived(char * msg) override; diff --git a/examples/placeholder/linux/InteractiveServer.cpp b/examples/placeholder/linux/InteractiveServer.cpp index 2ddd8da84c7ccf..c1356666747a70 100644 --- a/examples/placeholder/linux/InteractiveServer.cpp +++ b/examples/placeholder/linux/InteractiveServer.cpp @@ -226,7 +226,7 @@ bool InteractiveServer::Command(const chip::app::ConcreteCommandPath & path) auto valueStr = JsonToString(value); gInteractiveServerResult.MaybeAddResult(valueStr.c_str()); - LogErrorOnFailure(mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str())); + mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str()); gInteractiveServerResult.Reset(); return mIsReady; } @@ -243,7 +243,7 @@ bool InteractiveServer::ReadAttribute(const chip::app::ConcreteAttributePath & p auto valueStr = JsonToString(value); gInteractiveServerResult.MaybeAddResult(valueStr.c_str()); - LogErrorOnFailure(mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str())); + mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str()); gInteractiveServerResult.Reset(); return mIsReady; } @@ -260,7 +260,7 @@ bool InteractiveServer::WriteAttribute(const chip::app::ConcreteAttributePath & auto valueStr = JsonToString(value); gInteractiveServerResult.MaybeAddResult(valueStr.c_str()); - LogErrorOnFailure(mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str())); + mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str()); gInteractiveServerResult.Reset(); return mIsReady; } @@ -273,6 +273,6 @@ void InteractiveServer::CommissioningComplete() Json::Value value = Json::objectValue; auto valueStr = JsonToString(value); gInteractiveServerResult.MaybeAddResult(valueStr.c_str()); - LogErrorOnFailure(mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str())); + mWebSocketServer.Send(gInteractiveServerResult.AsJsonString().c_str()); gInteractiveServerResult.Reset(); }