From 95acf3d5c329d1e6a705a2543cba2734e6deeb7a Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 10 May 2023 10:20:48 -0400 Subject: [PATCH] Add a chip-tool command line argument for the storage directory. (#26452) * Add a chip-tool command line argument for the storage directory. 1) Adds --storage-directory argument for chip-tool. 2) Changes our CI test script to create temp directory for the storage, so running that script locally does not interfere with existing chip-tool configuration. * Make sure we don't pass the new storage args to darwin-framework-tool. --- .../chip-tool/commands/common/CHIPCommand.cpp | 6 +-- .../chip-tool/commands/common/CHIPCommand.h | 2 + examples/chip-tool/commands/common/Command.h | 11 ++++- .../chip-tool/commands/common/Commands.cpp | 27 +++++++++++-- examples/chip-tool/commands/common/Commands.h | 5 ++- .../interactive/InteractiveCommands.cpp | 2 +- scripts/tests/chiptest/test_definition.py | 28 +++++++------ scripts/tests/run_test_suite.py | 2 + src/controller/ExamplePersistentStorage.cpp | 40 +++++++++++-------- src/controller/ExamplePersistentStorage.h | 16 +++++++- 10 files changed, 96 insertions(+), 43 deletions(-) diff --git a/examples/chip-tool/commands/common/CHIPCommand.cpp b/examples/chip-tool/commands/common/CHIPCommand.cpp index d64db23eb072b2..ecf97505c331fe 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.cpp +++ b/examples/chip-tool/commands/common/CHIPCommand.cpp @@ -93,7 +93,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack() ReturnLogErrorOnFailure(chip::DeviceLayer::Internal::BLEMgrImpl().ConfigureBle(mBleAdapterId.ValueOr(0), true)); #endif - ReturnLogErrorOnFailure(mDefaultStorage.Init()); + ReturnLogErrorOnFailure(mDefaultStorage.Init(nullptr, GetStorageDirectory().ValueOr(nullptr))); ReturnLogErrorOnFailure(mOperationalKeystore.Init(&mDefaultStorage)); ReturnLogErrorOnFailure(mOpCertStore.Init(&mDefaultStorage)); @@ -326,7 +326,7 @@ CHIP_ERROR CHIPCommand::GetIdentityNodeId(std::string identity, chip::NodeId * n return CHIP_NO_ERROR; } - ReturnLogErrorOnFailure(mCommissionerStorage.Init(identity.c_str())); + ReturnLogErrorOnFailure(mCommissionerStorage.Init(identity.c_str(), GetStorageDirectory().ValueOr(nullptr))); *nodeId = mCommissionerStorage.GetLocalNodeId(); @@ -419,7 +419,7 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(CommissionerIdentity & identity, // TODO - OpCreds should only be generated for pairing command // store the credentials in persistent storage, and // generate when not available in the storage. - ReturnLogErrorOnFailure(mCommissionerStorage.Init(identity.mName.c_str())); + ReturnLogErrorOnFailure(mCommissionerStorage.Init(identity.mName.c_str(), GetStorageDirectory().ValueOr(nullptr))); if (mUseMaxSizedCerts.HasValue()) { auto option = CredentialIssuerCommands::CredentialIssuerOptions::kMaximizeCertificateSizes; diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 3f16ff95c4cd36..da0183aed04d89 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -86,6 +86,8 @@ class CHIPCommand : public Command AddArgument("trace_decode", 0, 1, &mTraceDecode); #endif // CHIP_CONFIG_TRANSPORT_TRACE_ENABLED AddArgument("ble-adapter", 0, UINT16_MAX, &mBleAdapterId); + AddArgument("storage-directory", &mStorageDirectory, + "Directory to place chip-tool's storage files in. Defaults to $TMPDIR, with fallback to /tmp"); } /////////// Command Interface ///////// diff --git a/examples/chip-tool/commands/common/Command.h b/examples/chip-tool/commands/common/Command.h index 39d59dfb1d6a44..c3c59f41f99477 100644 --- a/examples/chip-tool/commands/common/Command.h +++ b/examples/chip-tool/commands/common/Command.h @@ -260,12 +260,19 @@ class Command bool IsInteractive() { return mIsInteractive; } - CHIP_ERROR RunAsInteractive() + CHIP_ERROR RunAsInteractive(const chip::Optional & interactiveStorageDirectory) { - mIsInteractive = true; + mStorageDirectory = interactiveStorageDirectory; + mIsInteractive = true; return Run(); } + const chip::Optional & GetStorageDirectory() const { return mStorageDirectory; } + +protected: + // mStorageDirectory lives here so we can just set it in RunAsInteractive. + chip::Optional mStorageDirectory; + private: bool InitArgument(size_t argIndex, char * argValue); size_t AddArgument(const char * name, int64_t min, uint64_t max, void * out, ArgumentType type, const char * desc, diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp index b59aaf0ff47d59..eef7d8ccb39174 100644 --- a/examples/chip-tool/commands/common/Commands.cpp +++ b/examples/chip-tool/commands/common/Commands.cpp @@ -149,7 +149,7 @@ int Commands::Run(int argc, char ** argv) return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE; } -int Commands::RunInteractive(const char * command) +int Commands::RunInteractive(const char * command, const chip::Optional & storageDirectory) { std::vector arguments; VerifyOrReturnValue(DecodeArgumentsFromInteractiveMode(command, arguments), EXIT_FAILURE); @@ -174,7 +174,7 @@ int Commands::RunInteractive(const char * command) } ChipLogProgress(chipTool, "Command: %s", commandStr.c_str()); - auto err = RunCommand(argc, argv, true); + auto err = RunCommand(argc, argv, true, storageDirectory); // Do not delete arg[0] for (auto i = 1; i < argc; i++) @@ -185,7 +185,8 @@ int Commands::RunInteractive(const char * command) return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE; } -CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive) +CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive, + const chip::Optional & interactiveStorageDirectory) { Command * command = nullptr; @@ -271,7 +272,25 @@ CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive) return CHIP_ERROR_INVALID_ARGUMENT; } - return interactive ? command->RunAsInteractive() : command->Run(); + if (interactive) + { + return command->RunAsInteractive(interactiveStorageDirectory); + } + + // Now that the command is initialized, get our storage from it as needed + // and set up our loging level. +#ifdef CONFIG_USE_LOCAL_STORAGE + CHIP_ERROR err = mStorage.Init(nullptr, command->GetStorageDirectory().ValueOr(nullptr)); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Init Storage failure: %s", chip::ErrorStr(err)); + return err; + } + + chip::Logging::SetLogFilter(mStorage.GetLoggingLevel()); +#endif // CONFIG_USE_LOCAL_STORAGE + + return command->Run(); } Commands::ClusterMap::iterator Commands::GetCluster(std::string clusterName) diff --git a/examples/chip-tool/commands/common/Commands.h b/examples/chip-tool/commands/common/Commands.h index eae7946e84c5ef..f9ba9f739ae140 100644 --- a/examples/chip-tool/commands/common/Commands.h +++ b/examples/chip-tool/commands/common/Commands.h @@ -32,12 +32,13 @@ class Commands void Register(const char * clusterName, commands_list commandsList, const char * helpText = nullptr); int Run(int argc, char ** argv); - int RunInteractive(const char * command); + int RunInteractive(const char * command, const chip::Optional & storageDirectory = chip::NullOptional); private: using ClusterMap = std::map>; - CHIP_ERROR RunCommand(int argc, char ** argv, bool interactive = false); + CHIP_ERROR RunCommand(int argc, char ** argv, bool interactive = false, + const chip::Optional & interactiveStorageDirectory = chip::NullOptional); ClusterMap::iterator GetCluster(std::string clusterName); Command * GetCommand(CommandsVector & commands, std::string commandName); diff --git a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp index fc895d4eb69eb1..84e543afada9ba 100644 --- a/examples/chip-tool/commands/interactive/InteractiveCommands.cpp +++ b/examples/chip-tool/commands/interactive/InteractiveCommands.cpp @@ -319,7 +319,7 @@ bool InteractiveCommand::ParseCommand(char * command, int * status) ClearLine(); - *status = mHandler->RunInteractive(command); + *status = mHandler->RunInteractive(command, GetStorageDirectory()); return true; } diff --git a/scripts/tests/chiptest/test_definition.py b/scripts/tests/chiptest/test_definition.py index 5b0f4a548f505c..5009ad90109dae 100644 --- a/scripts/tests/chiptest/test_definition.py +++ b/scripts/tests/chiptest/test_definition.py @@ -15,6 +15,8 @@ import logging import os +import shutil +import tempfile import threading import time import typing @@ -227,6 +229,7 @@ class TestRunTime(Enum): CHIP_TOOL_BUILTIN = auto() # run via chip-tool built-in test commands CHIP_TOOL_PYTHON = auto() # use the python yaml test parser with chip-tool CHIP_REPL_PYTHON = auto() # use the python yaml test runner + DARWIN_FRAMEWORK_TOOL_BUILTIN = auto() # run via darwin-framework-tool built-in test commands @dataclass @@ -259,6 +262,8 @@ def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str, """ runner.capture_delegate = ExecutionCapture() + tool_storage_dir = None + try: if self.target == TestTarget.ALL_CLUSTERS: target_app = paths.all_clusters_app @@ -302,16 +307,8 @@ def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str, tool_cmd = paths.chip_tool if test_runtime != TestRunTime.CHIP_TOOL_PYTHON else paths.chip_tool_with_python_cmd - files_to_unlink = [ - '/tmp/chip_tool_config.ini', - '/tmp/chip_tool_config.alpha.ini', - '/tmp/chip_tool_config.beta.ini', - '/tmp/chip_tool_config.gamma.ini', - ] - - for f in files_to_unlink: - if os.path.exists(f): - os.unlink(f) + tool_storage_dir = tempfile.mkdtemp() + tool_storage_args = ['--storage-directory', tool_storage_dir] # Only start and pair the default app app = apps_register.get('default') @@ -319,8 +316,13 @@ def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str, pairing_cmd = tool_cmd + ['pairing', 'code', TEST_NODE_ID, app.setupCode] test_cmd = tool_cmd + ['tests', self.run_name] + ['--PICS', pics_file] if test_runtime == TestRunTime.CHIP_TOOL_PYTHON: - pairing_cmd += ['--server_path'] + [paths.chip_tool[-1]] - test_cmd += ['--server_path'] + [paths.chip_tool[-1]] + server_args = ['--server_path', paths.chip_tool[-1]] + \ + ['--server_arguments', 'interactive server ' + ' '.join(tool_storage_args)] + pairing_cmd += server_args + test_cmd += server_args + elif test_runtime == TestRunTime.CHIP_TOOL_BUILTIN: + pairing_cmd += tool_storage_args + test_cmd += tool_storage_args if dry_run: logging.info(" ".join(pairing_cmd)) @@ -348,3 +350,5 @@ def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str, apps_register.killAll() apps_register.factoryResetAll() apps_register.removeAll() + if tool_storage_dir is not None: + shutil.rmtree(tool_storage_dir, ignore_errors=True) diff --git a/scripts/tests/run_test_suite.py b/scripts/tests/run_test_suite.py index 3b70bd81d7cffe..53ce7da5d07a01 100755 --- a/scripts/tests/run_test_suite.py +++ b/scripts/tests/run_test_suite.py @@ -143,6 +143,8 @@ def main(context, dry_run, log_level, target, target_glob, target_skip_glob, runtime = TestRunTime.CHIP_REPL_PYTHON elif runner == 'chip_tool_python': runtime = TestRunTime.CHIP_TOOL_PYTHON + elif chip_tool is not None and os.path.basename(chip_tool) == "darwin-framework-tool": + runtime = TestRunTime.DARWIN_FRAMEWORK_TOOL_BUILTIN if chip_tool is None and not runtime == TestRunTime.CHIP_REPL_PYTHON: # non yaml tests REQUIRE chip-tool. Yaml tests should not require chip-tool diff --git a/src/controller/ExamplePersistentStorage.cpp b/src/controller/ExamplePersistentStorage.cpp index e4b8188780b816..b98b682141b244 100644 --- a/src/controller/ExamplePersistentStorage.cpp +++ b/src/controller/ExamplePersistentStorage.cpp @@ -39,37 +39,43 @@ constexpr const char kLocalNodeIdKey[] = "LocalNodeId"; constexpr const char kCommissionerCATsKey[] = "CommissionerCATs"; constexpr LogCategory kDefaultLoggingLevel = kLogCategory_Automation; -std::string GetFilename(const char * name) +std::string GetFilename(const char * directory, const char * name) { - const char * tmpdir = getenv("TMPDIR"); + const char * dir = directory; - if (tmpdir == nullptr) + if (dir == nullptr) { - tmpdir = "/tmp"; + dir = getenv("TMPDIR"); + } + + if (dir == nullptr) + { + dir = "/tmp"; } if (name == nullptr) { - return std::string(tmpdir) + "/chip_tool_config.ini"; + return std::string(dir) + "/chip_tool_config.ini"; } - return std::string(tmpdir) + "/chip_tool_config." + std::string(name) + ".ini"; + return std::string(dir) + "/chip_tool_config." + std::string(name) + ".ini"; } -CHIP_ERROR PersistentStorage::Init(const char * name) +CHIP_ERROR PersistentStorage::Init(const char * name, const char * directory) { CHIP_ERROR err = CHIP_NO_ERROR; std::ifstream ifs; - ifs.open(GetFilename(name), std::ifstream::in); + ifs.open(GetFilename(directory, name), std::ifstream::in); if (!ifs.good()) { - CommitConfig(name); - ifs.open(GetFilename(name), std::ifstream::in); + CommitConfig(directory, name); + ifs.open(GetFilename(directory, name), std::ifstream::in); } VerifyOrExit(ifs.is_open(), err = CHIP_ERROR_OPEN_FAILED); - mName = name; + mName = name; + mDirectory = directory; mConfig.parse(ifs); ifs.close(); @@ -125,7 +131,7 @@ CHIP_ERROR PersistentStorage::SyncSetKeyValue(const char * key, const void * val } mConfig.sections[kDefaultSectionName] = section; - return CommitConfig(mName); + return CommitConfig(mDirectory, mName); } CHIP_ERROR PersistentStorage::SyncDeleteKeyValue(const char * key) @@ -138,7 +144,7 @@ CHIP_ERROR PersistentStorage::SyncDeleteKeyValue(const char * key) section.erase(escapedKey); mConfig.sections[kDefaultSectionName] = section; - return CommitConfig(mName); + return CommitConfig(mDirectory, mName); } bool PersistentStorage::SyncDoesKeyExist(const char * key) @@ -173,15 +179,15 @@ CHIP_ERROR PersistentStorage::SyncClearAll() auto section = mConfig.sections[kDefaultSectionName]; section.clear(); mConfig.sections[kDefaultSectionName] = section; - return CommitConfig(mName); + return CommitConfig(mDirectory, mName); } -CHIP_ERROR PersistentStorage::CommitConfig(const char * name) +CHIP_ERROR PersistentStorage::CommitConfig(const char * directory, const char * name) { CHIP_ERROR err = CHIP_NO_ERROR; std::ofstream ofs; - std::string tmpPath = GetFilename(name) + ".tmp"; + std::string tmpPath = GetFilename(directory, name) + ".tmp"; ofs.open(tmpPath, std::ofstream::out | std::ofstream::trunc); VerifyOrExit(ofs.good(), err = CHIP_ERROR_WRITE_FAILED); @@ -189,7 +195,7 @@ CHIP_ERROR PersistentStorage::CommitConfig(const char * name) ofs.close(); VerifyOrExit(ofs.good(), err = CHIP_ERROR_WRITE_FAILED); - VerifyOrExit(rename(tmpPath.c_str(), GetFilename(name).c_str()) == 0, err = CHIP_ERROR_WRITE_FAILED); + VerifyOrExit(rename(tmpPath.c_str(), GetFilename(directory, name).c_str()) == 0, err = CHIP_ERROR_WRITE_FAILED); exit: return err; diff --git a/src/controller/ExamplePersistentStorage.h b/src/controller/ExamplePersistentStorage.h index f2afde99355bf7..9323c1de501b04 100644 --- a/src/controller/ExamplePersistentStorage.h +++ b/src/controller/ExamplePersistentStorage.h @@ -26,7 +26,18 @@ class PersistentStorage : public chip::PersistentStorageDelegate { public: - CHIP_ERROR Init(const char * name = nullptr); + /** + * name is the name of the storage to use. If null, defaults to + * "chip_tool_config.ini". + * + * directory is the directory the storage file should be placed in. If + * null, falls back to getenv("TMPDIR") and if that is not set falls back + * to /tmp. + * + * If non-null values are provided, the memory they point to is expected to + * outlive this object. + */ + CHIP_ERROR Init(const char * name = nullptr, const char * directory = nullptr); /////////// PersistentStorageDelegate Interface ///////// CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override; @@ -55,7 +66,8 @@ class PersistentStorage : public chip::PersistentStorageDelegate CHIP_ERROR SyncClearAll(); private: - CHIP_ERROR CommitConfig(const char * name); + CHIP_ERROR CommitConfig(const char * directory, const char * name); inipp::Ini mConfig; const char * mName; + const char * mDirectory; };