Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix all detectable thread races in chip-tool #7478

Merged
merged 3 commits into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 14 additions & 22 deletions examples/chip-tool/commands/clusters/ModelCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,39 +37,31 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
"Default DispatchSingleClusterCommand is called, this should be replaced by actual dispatched for cluster commands");
}

CHIP_ERROR ModelCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId)
CHIP_ERROR ModelCommand::Run(NodeId localId, NodeId remoteId)
{
CHIP_ERROR err = CHIP_NO_ERROR;

chip::Controller::CommissionerInitParams initParams;
initParams.storageDelegate = &storage;

err = mOpCredsIssuer.Initialize(storage);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", ErrorStr(err)));

initParams.operationalCredentialsDelegate = &mOpCredsIssuer;

err = mCommissioner.SetUdpListenPort(storage.GetListenPort());
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err)));
//
// Set this to true first BEFORE we send commands to ensure we don't
// end up in a situation where the response comes back faster than we can
// set the variable to true, which will cause it to block indefinitely.
//
UpdateWaitForResponse(true);

err = mCommissioner.Init(localId, initParams);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", ErrorStr(err)));
{
chip::DeviceLayer::StackLock lock;

err = mCommissioner.ServiceEvents();
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", ErrorStr(err)));
err = GetExecContext()->commissioner->GetDevice(remoteId, &mDevice);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId));

err = mCommissioner.GetDevice(remoteId, &mDevice);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Init failure! No pairing for device: %" PRIu64, localId));
err = SendCommand(mDevice, mEndPointId);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err)));
}

UpdateWaitForResponse(true);
err = SendCommand(mDevice, mEndPointId);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err)));
WaitForResponse(kWaitDurationInSeconds);

VerifyOrExit(GetCommandExitStatus(), err = CHIP_ERROR_INTERNAL);

exit:
mCommissioner.ServiceEventSignal();
mCommissioner.Shutdown();
return err;
}
4 changes: 1 addition & 3 deletions examples/chip-tool/commands/clusters/ModelCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ class ModelCommand : public Command
void AddArguments() { AddArgument("endpoint-id", CHIP_ZCL_ENDPOINT_MIN, CHIP_ZCL_ENDPOINT_MAX, &mEndPointId); }

/////////// Command Interface /////////
CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override;
CHIP_ERROR Run(NodeId localId, NodeId remoteId) override;

virtual CHIP_ERROR SendCommand(ChipDevice * device, uint8_t endPointId) = 0;

private:
ChipDeviceController mCommissioner;
ChipDevice * mDevice;
chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer;
uint8_t mEndPointId;
};
21 changes: 20 additions & 1 deletion examples/chip-tool/commands/common/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#pragma once

#include "controller/ExampleOperationalCredentialsIssuer.h"
#include <controller/CHIPDeviceController.h>
#include <inet/InetInterface.h>
#include <support/Span.h>
Expand Down Expand Up @@ -90,9 +91,23 @@ class Command
::chip::Inet::InterfaceId interfaceId;
};

/**
* @brief
* Encapsulates key objects in the CHIP stack that need continued
* access, so wrapping it in here makes it nice and compactly encapsulated.
*/
struct ExecutionContext
{
ChipDeviceCommissioner * commissioner;
chip::Controller::ExampleOperationalCredentialsIssuer * opCredsIssuer;
PersistentStorage * storage;
};

Command(const char * commandName) : mName(commandName) {}
virtual ~Command() {}

void SetExecutionContext(ExecutionContext & execContext) { mExecContext = &execContext; }

const char * GetName(void) const { return mName; }
const char * GetAttribute(void) const;
const char * GetArgumentName(size_t index) const;
Expand Down Expand Up @@ -147,7 +162,7 @@ class Command
return AddArgument(name, min, max, reinterpret_cast<void *>(out), Number_uint64);
}

virtual CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) = 0;
virtual CHIP_ERROR Run(NodeId localId, NodeId remoteId) = 0;

bool GetCommandExitStatus() const { return mCommandExitStatus; }
void SetCommandExitStatus(bool status)
Expand All @@ -159,6 +174,10 @@ class Command
void UpdateWaitForResponse(bool value);
void WaitForResponse(uint16_t duration);

protected:
ExecutionContext * GetExecContext() { return mExecContext; }
ExecutionContext * mExecContext;

private:
bool InitArgument(size_t argIndex, char * argValue);
size_t AddArgument(const char * name, int64_t min, uint64_t max, void * out, ArgumentType type);
Expand Down
57 changes: 48 additions & 9 deletions examples/chip-tool/commands/common/Commands.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void Commands::Register(const char * clusterName, commands_list commandsList)
int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
{
CHIP_ERROR err = CHIP_NO_ERROR;
PersistentStorage storage;
chip::Controller::CommissionerInitParams initParams;

err = chip::Platform::MemoryInit();
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init Memory failure: %s", chip::ErrorStr(err)));
Expand All @@ -51,19 +51,48 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
SuccessOrExit(err = chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(/* BLE adapter ID */ 0, /* BLE central */ true));
#endif

err = storage.Init();
err = mStorage.Init();
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init Storage failure: %s", chip::ErrorStr(err)));

chip::Logging::SetLogFilter(storage.GetLoggingLevel());
chip::Logging::SetLogFilter(mStorage.GetLoggingLevel());

err = RunCommand(storage, localId, remoteId, argc, argv);
initParams.storageDelegate = &mStorage;

err = mOpCredsIssuer.Initialize(mStorage);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Operational Cred Issuer: %s", chip::ErrorStr(err)));

initParams.operationalCredentialsDelegate = &mOpCredsIssuer;

err = mController.SetUdpListenPort(mStorage.GetListenPort());
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err)));

err = mController.Init(localId, initParams);
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err)));

err = mController.ServiceEvents();
mrjerryjohns marked this conversation as resolved.
Show resolved Hide resolved
VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", chip::ErrorStr(err)));

err = RunCommand(localId, remoteId, argc, argv);
SuccessOrExit(err);

exit:
#if CONFIG_DEVICE_LAYER
chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
mrjerryjohns marked this conversation as resolved.
Show resolved Hide resolved
#endif

//
// We can call DeviceController::Shutdown() safely without grabbing the stack lock
// since the CHIP thread and event queue have been stopped, preventing any thread
// races.
mrjerryjohns marked this conversation as resolved.
Show resolved Hide resolved
//
// TODO: This doesn't hold true on Darwin, issue #7557 tracks the problem.
//
mController.Shutdown();

return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE;
}

CHIP_ERROR Commands::RunCommand(PersistentStorage & storage, NodeId localId, NodeId remoteId, int argc, char ** argv)
CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv)
{
CHIP_ERROR err = CHIP_NO_ERROR;
std::map<std::string, CommandsVector>::iterator cluster;
Expand Down Expand Up @@ -125,11 +154,21 @@ CHIP_ERROR Commands::RunCommand(PersistentStorage & storage, NodeId localId, Nod
ExitNow(err = CHIP_ERROR_INVALID_ARGUMENT);
}

err = command->Run(storage, localId, remoteId);
if (err != CHIP_NO_ERROR)
{
ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err));
ExitNow();
Command::ExecutionContext execContext;

execContext.commissioner = &mController;
execContext.opCredsIssuer = &mOpCredsIssuer;
execContext.storage = &mStorage;

command->SetExecutionContext(execContext);

err = command->Run(localId, remoteId);
if (err != CHIP_NO_ERROR)
{
ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err));
ExitNow();
}
}

exit:
Expand Down
6 changes: 5 additions & 1 deletion examples/chip-tool/commands/common/Commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "../../config/PersistentStorage.h"
#include "Command.h"
#include <controller/ExampleOperationalCredentialsIssuer.h>
#include <map>

class Commands
Expand All @@ -32,7 +33,7 @@ class Commands
int Run(NodeId localId, NodeId remoteId, int argc, char ** argv);

private:
CHIP_ERROR RunCommand(PersistentStorage & storage, NodeId localId, NodeId remoteId, int argc, char ** argv);
CHIP_ERROR RunCommand(NodeId localId, NodeId remoteId, int argc, char ** argv);
std::map<std::string, CommandsVector>::iterator GetCluster(std::string clusterName);
Command * GetCommand(CommandsVector & commands, std::string commandName);
Command * GetGlobalCommand(CommandsVector & commands, std::string commandName, std::string attributeName);
Expand All @@ -44,4 +45,7 @@ class Commands
void ShowCommand(std::string executable, std::string clusterName, Command * command);

std::map<std::string, CommandsVector> mClusters;
chip::Controller::DeviceCommissioner mController;
chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer;
PersistentStorage mStorage;
};
4 changes: 2 additions & 2 deletions examples/chip-tool/commands/discover/Commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ class Update : public DiscoverCommand
CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) override
{
ChipDevice * device;
ReturnErrorOnFailure(mCommissioner.GetDevice(remoteId, &device));
ReturnErrorOnFailure(GetExecContext()->commissioner->GetDevice(remoteId, &device));
ChipLogProgress(chipTool, "Mdns: Updating NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", remoteId, fabricId);
return mCommissioner.UpdateDevice(device, fabricId);
return GetExecContext()->commissioner->UpdateDevice(device, fabricId);
}

/////////// DeviceAddressUpdateDelegate Interface /////////
Expand Down
32 changes: 19 additions & 13 deletions examples/chip-tool/commands/discover/DiscoverCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,33 @@

constexpr uint16_t kWaitDurationInSeconds = 30;

CHIP_ERROR DiscoverCommand::Run(PersistentStorage & storage, NodeId localId, NodeId remoteId)
CHIP_ERROR DiscoverCommand::Run(NodeId localId, NodeId remoteId)
{
chip::Controller::CommissionerInitParams params;
CHIP_ERROR err;

params.storageDelegate = &storage;
params.mDeviceAddressUpdateDelegate = this;
params.operationalCredentialsDelegate = &mOpCredsIssuer;
//
// Set this to true first BEFORE we send commands to ensure we don't
// end up in a situation where the response comes back faster than we can
// set the variable to true, which will cause it to block indefinitely.
//
UpdateWaitForResponse(true);

ReturnErrorOnFailure(mCommissioner.SetUdpListenPort(storage.GetListenPort()));
ReturnErrorOnFailure(mCommissioner.Init(localId, params));
ReturnErrorOnFailure(mCommissioner.ServiceEvents());
{
chip::DeviceLayer::StackLock lock;

ReturnErrorOnFailure(RunCommand(mNodeId, mFabricId));
GetExecContext()->commissioner->RegisterDeviceAddressUpdateDelegate(this);
err = RunCommand(mNodeId, mFabricId);
SuccessOrExit(err);
}

UpdateWaitForResponse(true);
WaitForResponse(kWaitDurationInSeconds);

mCommissioner.ServiceEventSignal();
mCommissioner.Shutdown();
exit:
if (err != CHIP_NO_ERROR)
{
return err;
}

VerifyOrReturnError(GetCommandExitStatus(), CHIP_ERROR_INTERNAL);

return CHIP_NO_ERROR;
}
6 changes: 1 addition & 5 deletions examples/chip-tool/commands/discover/DiscoverCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,11 @@ class DiscoverCommand : public Command, public chip::Controller::DeviceAddressUp
void OnAddressUpdateComplete(NodeId nodeId, CHIP_ERROR error) override{};

/////////// Command Interface /////////
CHIP_ERROR Run(PersistentStorage & storage, NodeId localId, NodeId remoteId) override;
CHIP_ERROR Run(NodeId localId, NodeId remoteId) override;

virtual CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) = 0;

protected:
ChipDeviceCommissioner mCommissioner;

private:
chip::NodeId mNodeId;
uint64_t mFabricId;
chip::Controller::ExampleOperationalCredentialsIssuer mOpCredsIssuer;
};
Loading