feat: Implement loading of extensions from config files #1471
Conversation
|
Hi @guanzi008. Thanks for your PR. I'm waiting for a OpenAtom-Linyaps member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
guanzi008
force-pushed
the
patch-3
branch
2 times, most recently
from
9c109b5 to
45aafe7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guanzi008 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guanzi008 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1) 全局扩展ll-cli config set-extensions --global org.demo.ExtA,org.demo.ExtB 2) 应用扩展覆盖ll-cli config set-extensions org.deepin.AppX org.demo.AppXExt 3) 环境变量ll-cli config set-env org.deepin.AppX LANG=zh_CN.UTF-8 PATH+=/opt/appx/bin 4) 文件系统映射ll-cli config add-fs org.deepin.AppX --host ~/Downloads --target /data --mode rw --persist 5) 命令设置ll-cli config set-command org.deepin.AppX myapp --entrypoint /opt/my/bin/myapp --cwd /work --args-prefix "--safe -v" --args-suffix "--log debug" QT_QPA_PLATFORM=wayland ll-cli config 支持维护沙箱 allowlist：新增 add-fs-allow、rm-fs-allow、clear-fs-allow 子命令， |
api/schema/v1.yaml
Outdated
| description: filesystem entries applied to command | ||
| items: | ||
| $ref: '#/$defs/LlCliFilesystemEntry' | ||
| args_prefix: |
There was a problem hiding this comment.
有一些配置项好像没用到，如果还没有实现功能，就先把配置项去掉
There was a problem hiding this comment.
这个是准备把配置写到应用的linglong.yaml
就是这里也可以写配置。
这只是我的一种尝试。
都没验证有没有效果。。
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: the ll-cli config command, which allows users to manage configurations for applications and bases. This enhances control over the runtime environment by enabling customization of extensions, environment variables, and filesystem mounts. The implementation includes schema updates, a dedicated parser for the new config command, and runtime logic to load and merge these configurations.
My review focuses on improving the robustness and maintainability of the new C++ code. The key suggestions include refactoring the manual command-line parsing to use the existing CLI11 framework, adding critical exception handling for user input, simplifying file I/O logic, and reducing code duplication. Addressing these points will make this valuable new feature more robust and easier to maintain.
apps/ll-cli/src/main.cpp
Outdated
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; |
There was a problem hiding this comment.
The use of std::stoul to parse an index from command-line arguments is not wrapped in a try-catch block. If the user provides an invalid number (e.g., a non-numeric string), std::stoul will throw an exception (std::invalid_argument or std::out_of_range), causing the program to terminate unexpectedly. Please add error handling to manage invalid input gracefully. For example:
try {
index = static_cast<size_t>(std::stoul(mainArgv[++i]));
} catch (const std::exception& e) {
fprintf(stderr, "Invalid index value: %s\n", mainArgv[i]);
return 1;
}
apps/ll-cli/src/main.cpp
Outdated
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; |
There was a problem hiding this comment.
The use of std::stoul to parse an index from command-line arguments is not wrapped in a try-catch block. If the user provides an invalid number (e.g., a non-numeric string), std::stoul will throw an exception (std::invalid_argument or std::out_of_range), causing the program to terminate unexpectedly. Please add error handling to manage invalid input gracefully. For example:
try {
index = static_cast<size_t>(std::stoul(mainArgv[++i]));
} catch (const std::exception& e) {
fprintf(stderr, "Invalid index value: %s\n", mainArgv[i]);
return 1;
}| } | ||
|
|
||
| static std::vector<std::string> splitCsv(const std::string &s) | ||
| { |
There was a problem hiding this comment.
The logic for retrying rename is confusing and potentially buggy. std::filesystem::rename should atomically replace the destination file if it exists. The retry logic with remove is unnecessary for atomic file updates within the same filesystem and can mask the original error. A single rename call is sufficient and clearer.
apps/ll-cli/src/main.cpp
Outdated
|
|
||
| auto parseScope = [&](int start) -> std::tuple<Scope, std::string, std::string, int> { | ||
| if (start >= argc) { | ||
| return { Scope::Global, "", "", start }; | ||
| } | ||
| std::string t = mainArgv[start]; | ||
| if (t == "--global") { | ||
| return { Scope::Global, "", "", start + 1 }; | ||
| } | ||
| if (t == "--base") { | ||
| if (start + 1 >= argc) { | ||
| fprintf(stderr, "--base requires <baseid>\n"); | ||
| return { Scope::Global, "", "", argc }; | ||
| } | ||
| return { Scope::Base, "", mainArgv[start + 1], start + 2 }; | ||
| } | ||
| return { Scope::App, t, "", start + 1 }; | ||
| }; | ||
|
|
||
| auto loadCliConfigFromPackage = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId) | ||
| -> std::optional<json> { | ||
| if (scope == Scope::Global) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| auto repoResult = initOSTreeRepo(); | ||
| if (!repoResult) { | ||
| qWarning() << "load cli config from package failed:" << repoResult.error(); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| auto *repoPtr = *repoResult; | ||
| auto list = repoPtr->listLocal(); | ||
| if (!list) { | ||
| qWarning() << "list local packages failed:" << list.error(); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| const auto matcher = [&](const linglong::api::types::v1::PackageInfoV2 &info) -> bool { | ||
| if (scope == Scope::App) { | ||
| return info.id == appId && info.kind == "app"; | ||
| } | ||
| if (scope == Scope::Base) { | ||
| return info.id == baseId && info.kind == "base"; | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| auto it = std::find_if(list->begin(), list->end(), matcher); | ||
| if (it == list->end() || !it->cliConfig) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| json packaged = *(it->cliConfig); | ||
| return packaged; | ||
| }; | ||
|
|
||
| auto openConfig = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId) -> std::optional<json> { | ||
| auto userPath = getConfigPath(scope, appId, baseId); | ||
| if (userPath.empty()) { | ||
| fprintf(stderr, "invalid config path\n"); | ||
| return std::nullopt; | ||
| } | ||
| bool userExists = false; | ||
| auto userJson = readJsonIfExists(userPath, &userExists); | ||
| if (!userJson) { | ||
| fprintf(stderr, "failed to read %s\n", userPath.string().c_str()); | ||
| return std::nullopt; | ||
| } | ||
| if (!userExists) { | ||
| auto searchPaths = getConfigSearchPaths(scope, appId, baseId); | ||
| for (size_t idx = 1; idx < searchPaths.size(); ++idx) { | ||
| bool existed = false; | ||
| auto fallback = readJsonIfExists(searchPaths[idx], &existed); | ||
| if (!fallback || !existed) { | ||
| continue; | ||
| } | ||
| return fallback; | ||
| } | ||
| if (auto packaged = loadCliConfigFromPackage(scope, appId, baseId)) { | ||
| return packaged; | ||
| } | ||
| } | ||
| return userJson; | ||
| }; | ||
| auto saveConfig = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId, | ||
| const json &j) -> bool { | ||
| auto path = getConfigPath(scope, appId, baseId); | ||
| if (path.empty()) { | ||
| return false; | ||
| } | ||
| if (!writeJsonAtomic(path, j)) { | ||
| fprintf(stderr, "failed to write %s\n", path.string().c_str()); | ||
| return false; | ||
| } | ||
| printf("Written %s\n", path.string().c_str()); | ||
| return true; | ||
| }; | ||
|
|
||
| if (sub == "set-extensions" || sub == "add-extensions") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> exts = splitCsv(mainArgv[i]); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetExtensions(*j, exts, sub == "set-extensions"); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "set-env") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> kvs; | ||
| for (; i < argc; ++i) { | ||
| kvs.push_back(mainArgv[i]); | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetEnv(*j, kvs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "unset-env") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> keys; | ||
| for (; i < argc; ++i) { | ||
| keys.push_back(mainArgv[i]); | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonUnsetEnv(*j, keys); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "add-fs") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| FsArg fs; | ||
| fs.mode = "ro"; | ||
| fs.persist = false; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--persist") { | ||
| fs.persist = true; | ||
| } else if (a == "--host" && i + 1 < argc) { | ||
| fs.host = mainArgv[++i]; | ||
| } else if (a == "--target" && i + 1 < argc) { | ||
| fs.target = mainArgv[++i]; | ||
| } else if (a == "--mode" && i + 1 < argc) { | ||
| fs.mode = mainArgv[++i]; | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (fs.host.empty() || fs.target.empty()) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonAddFs(*j, fs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "add-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| FsArg fs; | ||
| fs.mode = "ro"; | ||
| fs.persist = false; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--persist") { | ||
| fs.persist = true; | ||
| } else if (a == "--host" && i + 1 < argc) { | ||
| fs.host = mainArgv[++i]; | ||
| } else if (a == "--target" && i + 1 < argc) { | ||
| fs.target = mainArgv[++i]; | ||
| } else if (a == "--mode" && i + 1 < argc) { | ||
| fs.mode = mainArgv[++i]; | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (fs.host.empty() || fs.target.empty()) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonAddFsAllow(*j, fs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "rm-fs") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| std::optional<std::string> target; | ||
| std::optional<size_t> index; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--target" && i + 1 < argc) { | ||
| target = mainArgv[++i]; | ||
| } else if (a == "--index" && i + 1 < argc) { | ||
| index = static_cast<size_t>(std::stoul(mainArgv[++i])); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| bool ok = false; | ||
| if (target) { | ||
| ok = jsonRmFsByTarget(*j, *target); | ||
| } | ||
| if (!ok && index) { | ||
| ok = jsonRmFsByIndex(*j, *index); | ||
| } | ||
| if (!ok) { | ||
| fprintf(stderr, "no filesystem entry removed\n"); | ||
| return 1; | ||
| } | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "rm-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| std::optional<std::string> target; | ||
| std::optional<size_t> index; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--target" && i + 1 < argc) { | ||
| target = mainArgv[++i]; | ||
| } else if (a == "--index" && i + 1 < argc) { | ||
| index = static_cast<size_t>(std::stoul(mainArgv[++i])); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| bool ok = false; | ||
| if (target) { | ||
| ok = jsonRmFsAllowByTarget(*j, *target); | ||
| } | ||
| if (!ok && index) { | ||
| ok = jsonRmFsAllowByIndex(*j, *index); | ||
| } | ||
| if (!ok) { | ||
| fprintf(stderr, "no filesystem_allow entry removed\n"); | ||
| return 1; | ||
| } | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "clear-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonClearFsAllow(*j); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "set-command") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| CmdSetArg a; | ||
| a.cmd = mainArgv[i++]; | ||
| for (; i < argc; ++i) { | ||
| std::string t = mainArgv[i]; | ||
| if (t == "--entrypoint" && i + 1 < argc) { | ||
| a.entrypoint = mainArgv[++i]; | ||
| } else if (t == "--cwd" && i + 1 < argc) { | ||
| a.cwd = mainArgv[++i]; | ||
| } else if (t == "--args-prefix" && i + 1 < argc) { | ||
| std::stringstream ss(mainArgv[++i]); | ||
| std::string tok; | ||
| while (ss >> tok) { | ||
| a.argsPrefix.push_back(tok); | ||
| } | ||
| } else if (t == "--args-suffix" && i + 1 < argc) { | ||
| std::stringstream ss(mainArgv[++i]); | ||
| std::string tok; | ||
| while (ss >> tok) { | ||
| a.argsSuffix.push_back(tok); | ||
| } | ||
| } else if (t.find('=') != std::string::npos) { | ||
| a.envKVs.push_back(t); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", t.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetCommand(*j, a); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "unset-command") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::string cmd = mainArgv[i]; | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonUnsetCommand(*j, cmd); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| } | ||
| // ===== end: "ll-cli config ..." dispatcher ===== | ||
|
|
||
| CLI::App commandParser{ _( | ||
| "linyaps CLI\n" | ||
| "A CLI program to run application and manage application and runtime\n") }; | ||
| auto shortConfigHelp = configShortHelp(); | ||
| auto fullConfigHelp = shortConfigHelp + configUsageLines(); | ||
| commandParser.formatter(std::make_shared<ConfigAwareFormatter>(shortConfigHelp, |
There was a problem hiding this comment.
The implementation of the config subcommand and its options is done through a large block of manual argument parsing, separate from the CLI11 library used for other commands. This approach is less maintainable and more error-prone than using the features of the CLI library. It would be better to define the config command and its subcommands using CLI11 to ensure consistent behavior, better error handling, and improved maintainability. This would also centralize all command-line parsing logic.
| if (!std::filesystem::exists(hostPath, ec)) { | ||
| ec.clear(); | ||
| if (!std::filesystem::create_directories(hostPath, ec) && ec) { | ||
| ec.clear(); | ||
| auto parent = hostPath.parent_path(); | ||
| if (!parent.empty()) { | ||
| std::filesystem::create_directories(parent, ec); | ||
| } | ||
| } | ||
| } | ||
| if (ec || !std::filesystem::exists(hostPath, ec)) { | ||
| qWarning() << "failed to prepare persist directory for" | ||
| << QString::fromStdString(host) << ":" << ec.message().c_str(); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic to ensure the persistent directory exists is overly complicated and contains buggy error handling. std::filesystem::create_directories is recursive, so there's no need for a fallback to create parent directories. The usage of std::error_code is also incorrect, as it's not cleared before every call that can set it. The entire block can be simplified significantly.
std::error_code ec;
std::filesystem::create_directories(hostPath, ec);
if (ec) {
qWarning() << "failed to prepare persist directory for"
<< QString::fromStdString(host) << ":" << ec.message().c_str();
continue;
}| // 1) common env | ||
| { | ||
| std::vector<std::string> envKVs; | ||
| collectEnvFromJson(mergedCfg, envKVs); | ||
| if (!envKVs.empty()) { | ||
| std::map<std::string, std::string> genEnv; | ||
| std::string basePath; | ||
| if (auto sysPath = ::getenv("PATH")) { | ||
| basePath = sysPath; | ||
| } | ||
| auto extPathIt = environment.find("PATH"); | ||
| for (const auto &kv : envKVs) { | ||
| auto pos = kv.find("+="); | ||
| if (pos != std::string::npos) { | ||
| auto key = kv.substr(0, pos); | ||
| auto add = kv.substr(pos + 2); | ||
| if (key == "PATH") { | ||
| if (genEnv.count("PATH")) { | ||
| genEnv["PATH"] += ":" + add; | ||
| } else if (extPathIt != environment.end()) { | ||
| genEnv["PATH"] = | ||
| extPathIt->second.empty() ? add : extPathIt->second + ":" + add; | ||
| } else if (!basePath.empty()) { | ||
| genEnv["PATH"] = basePath + ":" + add; | ||
| } else { | ||
| genEnv["PATH"] = add; | ||
| } | ||
| } else { | ||
| qWarning() << "ignore '+=' env for key:" << QString::fromStdString(key); | ||
| } | ||
| continue; | ||
| } | ||
| auto eq = kv.find('='); | ||
| if (eq == std::string::npos) { | ||
| continue; | ||
| } | ||
| genEnv[kv.substr(0, eq)] = kv.substr(eq + 1); | ||
| } | ||
| if (!genEnv.empty()) { | ||
| if (auto it = genEnv.find("PATH"); it != genEnv.end()) { | ||
| mergedPath = it->second; | ||
| } | ||
| builder.appendEnv(genEnv); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 2) common filesystem | ||
| { | ||
| const auto &fsPolicy = filesystemPolicy(); | ||
| if (fsPolicy.allowListConfigured) { | ||
| if (!fsPolicy.allowList.empty()) { | ||
| auto allowList = fsPolicy.allowList; | ||
| builder.addExtraMounts(std::move(allowList)); | ||
| } | ||
| } else if (!fsPolicy.extra.empty()) { | ||
| auto extraMounts = fsPolicy.extra; | ||
| builder.addExtraMounts(std::move(extraMounts)); | ||
| } | ||
| } | ||
| // === end: merge Global->Base->App config === | ||
|
|
||
| if (!environment.empty()) { | ||
| if (auto it = environment.find("PATH"); it != environment.end()) { | ||
| mergedPath = it->second; | ||
| } | ||
| builder.appendEnv(environment); | ||
| } | ||
|
|
||
| // === begin: command-level settings (highest priority) === | ||
| { | ||
| std::string execName = currentAppId; | ||
| if (!execName.empty()) { | ||
| if (const json *node = pickCommandNode(mergedCfg, execName)) { | ||
| CommandSettings cs = parseCommandSettings(currentAppId, *node); | ||
|
|
||
| if (!cs.envKVs.empty()) { | ||
| std::map<std::string, std::string> cmdEnv; | ||
| std::string basePath; | ||
| if (auto sysPath = ::getenv("PATH")) { | ||
| basePath = sysPath; | ||
| } | ||
| auto extPathIt = environment.find("PATH"); | ||
| for (const auto &kv : cs.envKVs) { | ||
| auto posp = kv.find("+="); | ||
| if (posp != std::string::npos) { | ||
| auto key = kv.substr(0, posp); | ||
| auto add = kv.substr(posp + 2); | ||
| if (key == "PATH") { | ||
| if (cmdEnv.count("PATH")) { | ||
| cmdEnv["PATH"] += ":" + add; | ||
| } else if (mergedPath) { | ||
| cmdEnv["PATH"] = | ||
| mergedPath->empty() ? add : *mergedPath + ":" + add; | ||
| } else if (extPathIt != environment.end()) { | ||
| cmdEnv["PATH"] = | ||
| extPathIt->second.empty() | ||
| ? add | ||
| : extPathIt->second + ":" + add; | ||
| } else if (!basePath.empty()) { | ||
| cmdEnv["PATH"] = basePath + ":" + add; | ||
| } else { | ||
| cmdEnv["PATH"] = add; | ||
| } | ||
| } else { | ||
| qWarning() << "ignore '+=' env for key in command settings:" | ||
| << QString::fromStdString(key); | ||
| } | ||
| continue; | ||
| } | ||
| auto eq = kv.find('='); | ||
| if (eq == std::string::npos) { | ||
| continue; | ||
| } | ||
| cmdEnv[kv.substr(0, eq)] = kv.substr(eq + 1); | ||
| } | ||
| if (!cmdEnv.empty()) { | ||
| if (auto it = cmdEnv.find("PATH"); it != cmdEnv.end()) { | ||
| mergedPath = it->second; | ||
| } | ||
| builder.appendEnv(cmdEnv); | ||
| } | ||
| } | ||
|
|
||
| if (!cs.mounts.empty()) builder.addExtraMounts(cs.mounts); | ||
| // TODO: when builder exposes API for entrypoint/cwd/args, apply here as well. | ||
| } | ||
| } | ||
| } | ||
| // === end: command-level settings === |
There was a problem hiding this comment.
The logic for merging the PATH environment variable is duplicated in the common environment setup and again in the command-level settings block. This duplication makes the code harder to maintain. Consider extracting this logic into a helper function that takes the existing PATH components and the new components to be added, and returns the merged PATH string. This would improve readability and reduce redundancy.
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful configuration system for ll-cli, allowing users to customize environments, filesystem mounts, and command behavior. The implementation is extensive, touching schema definitions, CLI argument parsing, and runtime context creation. The core logic for merging configurations from different sources (global, base, app, and package-provided) is well-thought-out. However, the manual command-line parsing for the new config command is a significant maintainability concern and should be refactored. There are also some correctness and efficiency issues that need to be addressed.
apps/ll-cli/src/main.cpp
Outdated
| printConfigUsage(); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
std::stoul can throw std::invalid_argument or std::out_of_range if the input string is not a valid number, which will cause the program to terminate. You should wrap this call in a try-catch block to handle potential exceptions gracefully and provide a user-friendly error message.
} else if (a == "--index" && i + 1 < argc) {
try {
index = static_cast<size_t>(std::stoul(mainArgv[++i]));
} catch (const std::exception &e) {
fprintf(stderr, "Invalid index value: %s\n", mainArgv[i]);
return 1;
}
}
apps/ll-cli/src/main.cpp
Outdated
| printConfigUsage(); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
std::stoul can throw an exception if the input is not a valid number, which will crash the application. This should be wrapped in a try-catch block for robust error handling.
} else if (a == "--index" && i + 1 < argc) {
try {
index = static_cast<size_t>(std::stoul(mainArgv[++i]));
} catch (const std::exception &e) {
fprintf(stderr, "Invalid index value: %s\n", mainArgv[i]);
return 1;
}
}
apps/ll-cli/src/main.cpp
Outdated
|
|
||
| auto parseScope = [&](int start) -> std::tuple<Scope, std::string, std::string, int> { | ||
| if (start >= argc) { | ||
| return { Scope::Global, "", "", start }; | ||
| } | ||
| std::string t = mainArgv[start]; | ||
| if (t == "--global") { | ||
| return { Scope::Global, "", "", start + 1 }; | ||
| } | ||
| if (t == "--base") { | ||
| if (start + 1 >= argc) { | ||
| fprintf(stderr, "--base requires <baseid>\n"); | ||
| return { Scope::Global, "", "", argc }; | ||
| } | ||
| return { Scope::Base, "", mainArgv[start + 1], start + 2 }; | ||
| } | ||
| return { Scope::App, t, "", start + 1 }; | ||
| }; | ||
|
|
||
| auto loadCliConfigFromPackage = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId) | ||
| -> std::optional<json> { | ||
| if (scope == Scope::Global) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| auto repoResult = initOSTreeRepo(); | ||
| if (!repoResult) { | ||
| qWarning() << "load cli config from package failed:" << repoResult.error(); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| auto *repoPtr = *repoResult; | ||
| auto list = repoPtr->listLocal(); | ||
| if (!list) { | ||
| qWarning() << "list local packages failed:" << list.error(); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| const auto matcher = [&](const linglong::api::types::v1::PackageInfoV2 &info) -> bool { | ||
| if (scope == Scope::App) { | ||
| return info.id == appId && info.kind == "app"; | ||
| } | ||
| if (scope == Scope::Base) { | ||
| return info.id == baseId && info.kind == "base"; | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| auto it = std::find_if(list->begin(), list->end(), matcher); | ||
| if (it == list->end() || !it->cliConfig) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| json packaged = *(it->cliConfig); | ||
| return packaged; | ||
| }; | ||
|
|
||
| auto openConfig = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId) -> std::optional<json> { | ||
| auto userPath = getConfigPath(scope, appId, baseId); | ||
| if (userPath.empty()) { | ||
| fprintf(stderr, "invalid config path\n"); | ||
| return std::nullopt; | ||
| } | ||
| bool userExists = false; | ||
| auto userJson = readJsonIfExists(userPath, &userExists); | ||
| if (!userJson) { | ||
| fprintf(stderr, "failed to read %s\n", userPath.string().c_str()); | ||
| return std::nullopt; | ||
| } | ||
| if (!userExists) { | ||
| auto searchPaths = getConfigSearchPaths(scope, appId, baseId); | ||
| for (size_t idx = 1; idx < searchPaths.size(); ++idx) { | ||
| bool existed = false; | ||
| auto fallback = readJsonIfExists(searchPaths[idx], &existed); | ||
| if (!fallback || !existed) { | ||
| continue; | ||
| } | ||
| return fallback; | ||
| } | ||
| if (auto packaged = loadCliConfigFromPackage(scope, appId, baseId)) { | ||
| return packaged; | ||
| } | ||
| } | ||
| return userJson; | ||
| }; | ||
| auto saveConfig = [&](Scope scope, | ||
| const std::string &appId, | ||
| const std::string &baseId, | ||
| const json &j) -> bool { | ||
| auto path = getConfigPath(scope, appId, baseId); | ||
| if (path.empty()) { | ||
| return false; | ||
| } | ||
| if (!writeJsonAtomic(path, j)) { | ||
| fprintf(stderr, "failed to write %s\n", path.string().c_str()); | ||
| return false; | ||
| } | ||
| printf("Written %s\n", path.string().c_str()); | ||
| return true; | ||
| }; | ||
|
|
||
| if (sub == "set-extensions" || sub == "add-extensions") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> exts = splitCsv(mainArgv[i]); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetExtensions(*j, exts, sub == "set-extensions"); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "set-env") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> kvs; | ||
| for (; i < argc; ++i) { | ||
| kvs.push_back(mainArgv[i]); | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetEnv(*j, kvs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "unset-env") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::vector<std::string> keys; | ||
| for (; i < argc; ++i) { | ||
| keys.push_back(mainArgv[i]); | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonUnsetEnv(*j, keys); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "add-fs") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| FsArg fs; | ||
| fs.mode = "ro"; | ||
| fs.persist = false; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--persist") { | ||
| fs.persist = true; | ||
| } else if (a == "--host" && i + 1 < argc) { | ||
| fs.host = mainArgv[++i]; | ||
| } else if (a == "--target" && i + 1 < argc) { | ||
| fs.target = mainArgv[++i]; | ||
| } else if (a == "--mode" && i + 1 < argc) { | ||
| fs.mode = mainArgv[++i]; | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (fs.host.empty() || fs.target.empty()) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonAddFs(*j, fs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "add-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| FsArg fs; | ||
| fs.mode = "ro"; | ||
| fs.persist = false; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--persist") { | ||
| fs.persist = true; | ||
| } else if (a == "--host" && i + 1 < argc) { | ||
| fs.host = mainArgv[++i]; | ||
| } else if (a == "--target" && i + 1 < argc) { | ||
| fs.target = mainArgv[++i]; | ||
| } else if (a == "--mode" && i + 1 < argc) { | ||
| fs.mode = mainArgv[++i]; | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (fs.host.empty() || fs.target.empty()) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonAddFsAllow(*j, fs); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "rm-fs") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| std::optional<std::string> target; | ||
| std::optional<size_t> index; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--target" && i + 1 < argc) { | ||
| target = mainArgv[++i]; | ||
| } else if (a == "--index" && i + 1 < argc) { | ||
| index = static_cast<size_t>(std::stoul(mainArgv[++i])); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| bool ok = false; | ||
| if (target) { | ||
| ok = jsonRmFsByTarget(*j, *target); | ||
| } | ||
| if (!ok && index) { | ||
| ok = jsonRmFsByIndex(*j, *index); | ||
| } | ||
| if (!ok) { | ||
| fprintf(stderr, "no filesystem entry removed\n"); | ||
| return 1; | ||
| } | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "rm-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| std::optional<std::string> target; | ||
| std::optional<size_t> index; | ||
| for (; i < argc; ++i) { | ||
| std::string a = mainArgv[i]; | ||
| if (a == "--target" && i + 1 < argc) { | ||
| target = mainArgv[++i]; | ||
| } else if (a == "--index" && i + 1 < argc) { | ||
| index = static_cast<size_t>(std::stoul(mainArgv[++i])); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", a.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| if (!target && !index) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| bool ok = false; | ||
| if (target) { | ||
| ok = jsonRmFsAllowByTarget(*j, *target); | ||
| } | ||
| if (!ok && index) { | ||
| ok = jsonRmFsAllowByIndex(*j, *index); | ||
| } | ||
| if (!ok) { | ||
| fprintf(stderr, "no filesystem_allow entry removed\n"); | ||
| return 1; | ||
| } | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "clear-fs-allow") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonClearFsAllow(*j); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "set-command") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| CmdSetArg a; | ||
| a.cmd = mainArgv[i++]; | ||
| for (; i < argc; ++i) { | ||
| std::string t = mainArgv[i]; | ||
| if (t == "--entrypoint" && i + 1 < argc) { | ||
| a.entrypoint = mainArgv[++i]; | ||
| } else if (t == "--cwd" && i + 1 < argc) { | ||
| a.cwd = mainArgv[++i]; | ||
| } else if (t == "--args-prefix" && i + 1 < argc) { | ||
| std::stringstream ss(mainArgv[++i]); | ||
| std::string tok; | ||
| while (ss >> tok) { | ||
| a.argsPrefix.push_back(tok); | ||
| } | ||
| } else if (t == "--args-suffix" && i + 1 < argc) { | ||
| std::stringstream ss(mainArgv[++i]); | ||
| std::string tok; | ||
| while (ss >> tok) { | ||
| a.argsSuffix.push_back(tok); | ||
| } | ||
| } else if (t.find('=') != std::string::npos) { | ||
| a.envKVs.push_back(t); | ||
| } else { | ||
| fprintf(stderr, "unknown arg: %s\n", t.c_str()); | ||
| return 1; | ||
| } | ||
| } | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonSetCommand(*j, a); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
| if (sub == "unset-command") { | ||
| auto [scope, appId, baseId, i] = parseScope(3); | ||
| if (i >= argc) { | ||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| std::string cmd = mainArgv[i]; | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| return 1; | ||
| } | ||
| jsonUnsetCommand(*j, cmd); | ||
| if (!saveConfig(scope, appId, baseId, *j)) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| printConfigUsage(); | ||
| return 1; | ||
| } | ||
| } | ||
| // ===== end: "ll-cli config ..." dispatcher ===== | ||
|
|
||
| CLI::App commandParser{ _( | ||
| "linyaps CLI\n" | ||
| "A CLI program to run application and manage application and runtime\n") }; | ||
| auto shortConfigHelp = configShortHelp(); | ||
| auto fullConfigHelp = shortConfigHelp + configUsageLines(); | ||
| commandParser.formatter(std::make_shared<ConfigAwareFormatter>(shortConfigHelp, | ||
| fullConfigHelp, |
There was a problem hiding this comment.
The entire config subcommand and its arguments are parsed manually. This large block of code is difficult to read, maintain, and extend. It would be much better to integrate this as a subcommand within the CLI11 framework, which is already used for other commands. This would simplify argument parsing, improve readability, and make the help messages more consistent.
| if (std::find(result.begin(), result.end(), ext) == result.end()) { | ||
| result.emplace_back(std::move(ext)); | ||
| } |
There was a problem hiding this comment.
Using std::find on a std::vector inside a loop to check for duplicates has a time complexity of O(N), making the overall loop O(N^2). For better performance, you could use a std::unordered_set to keep track of the extensions that have already been added. Also, the comment on line 76 is in Chinese and should be translated to English for consistency.
| if (std::find(result.begin(), result.end(), ext) == result.end()) { | ||
| result.emplace_back(std::move(ext)); | ||
| } |
There was a problem hiding this comment.
Using std::find on a std::vector inside a loop to check for duplicates is inefficient (O(N^2) complexity). Using a std::unordered_set to track existing extensions would be more performant.
| std::error_code ec; | ||
| std::filesystem::path hostPath(host); | ||
| if (!std::filesystem::exists(hostPath, ec)) { | ||
| ec.clear(); | ||
| if (!std::filesystem::create_directories(hostPath, ec) && ec) { | ||
| ec.clear(); | ||
| auto parent = hostPath.parent_path(); | ||
| if (!parent.empty()) { | ||
| std::filesystem::create_directories(parent, ec); | ||
| } | ||
| } | ||
| } | ||
| if (ec || !std::filesystem::exists(hostPath, ec)) { | ||
| qWarning() << "failed to prepare persist directory for" | ||
| << QString::fromStdString(host) << ":" << ec.message().c_str(); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic to create directories for persistent mounts is overly complex. std::filesystem::create_directories already creates all necessary parent directories, so the nested checks and attempts to create parent directories separately are redundant. This can be simplified to a single call to create_directories followed by error checking.
std::error_code ec;
std::filesystem::path hostPath(host);
std::filesystem::create_directories(hostPath, ec);
if (ec) {
qWarning() << "failed to prepare persist directory for"
<< QString::fromStdString(host) << ":" << ec.message().c_str();
continue;
}| // 1) common env | ||
| { | ||
| std::vector<std::string> envKVs; | ||
| collectEnvFromJson(mergedCfg, envKVs); | ||
| if (!envKVs.empty()) { | ||
| std::map<std::string, std::string> genEnv; | ||
| std::string basePath; | ||
| if (auto sysPath = ::getenv("PATH")) { | ||
| basePath = sysPath; | ||
| } | ||
| auto extPathIt = environment.find("PATH"); | ||
| for (const auto &kv : envKVs) { | ||
| auto pos = kv.find("+="); | ||
| if (pos != std::string::npos) { | ||
| auto key = kv.substr(0, pos); | ||
| auto add = kv.substr(pos + 2); | ||
| if (key == "PATH") { | ||
| if (genEnv.count("PATH")) { | ||
| genEnv["PATH"] += ":" + add; | ||
| } else if (extPathIt != environment.end()) { | ||
| genEnv["PATH"] = | ||
| extPathIt->second.empty() ? add : extPathIt->second + ":" + add; | ||
| } else if (!basePath.empty()) { | ||
| genEnv["PATH"] = basePath + ":" + add; | ||
| } else { | ||
| genEnv["PATH"] = add; | ||
| } | ||
| } else { | ||
| qWarning() << "ignore '+=' env for key:" << QString::fromStdString(key); | ||
| } | ||
| continue; | ||
| } | ||
| auto eq = kv.find('='); | ||
| if (eq == std::string::npos) { | ||
| continue; | ||
| } | ||
| genEnv[kv.substr(0, eq)] = kv.substr(eq + 1); | ||
| } | ||
| if (!genEnv.empty()) { | ||
| if (auto it = genEnv.find("PATH"); it != genEnv.end()) { | ||
| mergedPath = it->second; | ||
| } | ||
| builder.appendEnv(genEnv); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling PATH variable concatenation is complex and is duplicated later in the file (lines 1090-1136). This should be extracted into a helper function to improve readability and reduce code duplication. The function could take the current environment maps and the key-value pairs, and return the updated environment map.
guanzi008
force-pushed
the
patch-3
branch
4 times, most recently
from
927e569 to
b4e3653
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature for managing application configurations via ll-cli config commands and loading extensions from these configuration files at runtime. The implementation is extensive, adding JSON-based configuration handling, command-line interface extensions using CLI11, and runtime logic to apply these configurations. My review focuses on improving maintainability by reducing code duplication, enhancing error handling, and making the CLI more robust and user-friendly. Overall, this is a solid addition, and the feedback aims to refine the implementation.
| static std::optional<json> readJsonIfExists(const std::filesystem::path &p, bool *existed = nullptr) | ||
| { | ||
| try { | ||
| std::error_code ec; | ||
| if (!std::filesystem::exists(p, ec)) { | ||
| if (existed) { | ||
| *existed = false; | ||
| } | ||
| return json::object(); | ||
| } | ||
| std::ifstream in(p); | ||
| if (!in.is_open()) { | ||
| return std::nullopt; | ||
| } | ||
| if (existed) { | ||
| *existed = true; | ||
| } | ||
| json j; | ||
| in >> j; | ||
| return j.is_null() ? json::object() : j; | ||
| } catch (...) { | ||
| return std::nullopt; | ||
| } | ||
| } |
There was a problem hiding this comment.
The catch (...) block is too broad. It catches all exceptions, including system errors, which can hide important issues and make debugging difficult. It's better to catch specific exceptions, like nlohmann::json::exception for JSON parsing errors, and log a more informative message.
} catch (const json::exception& e) {
fprintf(stderr, "JSON parsing error in %s: %s\n", p.string().c_str(), e.what());
return std::nullopt;
} catch (const std::exception& e) {
fprintf(stderr, "Error reading %s: %s\n", p.string().c_str(), e.what());
return std::nullopt;
}| if (!a.argsPrefix.empty()) { | ||
| auto &arr = node["args_prefix"]; | ||
| arr = json::array(); | ||
| for (auto &s : a.argsPrefix) { | ||
| arr.push_back(s); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code for populating JSON arrays from vectors can be simplified. The nlohmann::json library can directly convert std::vector<std::string> to a JSON array, which makes the code more concise and readable.
if (!a.argsPrefix.empty()) {
node["args_prefix"] = a.argsPrefix;
}| if (!a.envKVs.empty()) { | ||
| auto &env = node["env"]; | ||
| if (!env.is_object()) { | ||
| env = json::object(); | ||
| } | ||
| for (auto &kv : a.envKVs) { | ||
| auto pos = kv.find('='); | ||
| if (pos == std::string::npos) { | ||
| continue; | ||
| } | ||
| auto k = trim(kv.substr(0, pos)); | ||
| auto v = kv.substr(pos + 1); | ||
| if (!k.empty()) { | ||
| env[k] = v; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for parsing and setting environment variables from a vector of "KEY=VALUE" strings is duplicated here and in the jsonSetEnv function. This duplicated code should be extracted into a common helper function to improve maintainability and reduce redundancy.
| static std::optional<json> | ||
| loadCliConfigFromPackage(Scope scope, const std::string &appId, const std::string &baseId) | ||
| { | ||
| Q_UNUSED(scope); | ||
| Q_UNUSED(appId); | ||
| Q_UNUSED(baseId); | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
This function loadCliConfigFromPackage is a stub and always returns std::nullopt. If loading configuration from a package is a planned feature, it should be implemented. If not, this stub and its call site in openConfig should be removed to avoid dead code.
| { | ||
| auto userPath = getConfigPath(scope, appId, baseId); | ||
| if (userPath.empty()) { | ||
| fprintf(stderr, "invalid config path\n"); |
There was a problem hiding this comment.
This C++ file uses the C-style I/O function fprintf. For consistency with the rest of the C++ codebase, which uses std::cout, it's better to use C++ streams like std::cerr for error reporting.
std::cerr << "invalid config path\n";| auto scopeOpts = std::make_shared<ConfigScopeOptions>(); | ||
| addConfigScopeOptions(sub, *scopeOpts); | ||
| auto fs = std::make_shared<FsArg>(); | ||
| fs->mode = "ro"; |
There was a problem hiding this comment.
This line is redundant. The default_str("ro") call on the --mode option already ensures that fs->mode will be "ro" if the option is not provided by the user. Removing this line makes the code cleaner.
apps/ll-cli/src/main.cpp
Outdated
| sub->add_option("--target", *target, _("Target path inside container")); | ||
| sub->add_option("--index", *index, _("Index of entry in list")); |
There was a problem hiding this comment.
The --target and --index options are meant to be mutually exclusive, as suggested by the help text (--target PATH | --index N). However, the current implementation doesn't enforce this, leading to potentially confusing behavior. It's better to use CLI11's excludes() feature to enforce mutual exclusion and require_option(1) to ensure at least one is provided. This will make the command-line interface more robust and user-friendly. The manual check in the callback can then be removed.
auto *targetOpt = sub->add_option("--target", *target, _("Target path inside container"));
auto *indexOpt = sub->add_option("--index", *index, _("Index of entry in list"));
targetOpt->excludes(indexOpt);
sub->require_option(1);| static std::vector<std::filesystem::path> configBasesUserFirst() | ||
| { | ||
| namespace fs = std::filesystem; | ||
| std::vector<fs::path> bases; | ||
| std::unordered_set<std::string> seen; | ||
|
|
||
| auto addBase = [&](const fs::path &candidate) { | ||
| if (candidate.empty()) { | ||
| return; | ||
| } | ||
| auto normalized = candidate.lexically_normal(); | ||
| auto key = normalized.string(); | ||
| if (key.empty()) { | ||
| return; | ||
| } | ||
| if (seen.insert(key).second) { | ||
| bases.emplace_back(std::move(normalized)); | ||
| } | ||
| }; | ||
|
|
||
| if (const char *xdgConfigHome = ::getenv("XDG_CONFIG_HOME"); | ||
| xdgConfigHome && xdgConfigHome[0] != '\0') { | ||
| addBase(fs::path(xdgConfigHome) / "linglong"); | ||
| } else if (const char *homeEnv = ::getenv("HOME"); homeEnv && homeEnv[0] != '\0') { | ||
| addBase(fs::path(homeEnv) / ".config" / "linglong"); | ||
| } | ||
|
|
||
| addBase(fs::path(LINGLONG_DATA_DIR) / "config"); | ||
| return bases; | ||
| } |
There was a problem hiding this comment.
There is significant code duplication for configuration file handling logic between this file (e.g., configBasesUserFirst) and apps/ll-cli/src/main.cpp (e.g., getBaseConfigDir). This duplicated code for discovering configuration directories should be extracted into a shared utility function to improve maintainability and reduce redundancy.
| try { | ||
| if (!fs::exists(p)) { | ||
| return; | ||
| } | ||
| std::ifstream in(p); | ||
| if (!in.is_open()) { | ||
| return; | ||
| } | ||
| nlohmann::json j; | ||
| in >> j; | ||
| if (!j.contains("extensions") || !j.at("extensions").is_array()) { | ||
| return; | ||
| } | ||
| for (const auto &elem : j.at("extensions")) { | ||
| if (elem.is_string()) { | ||
| addExtension(elem.get<std::string>()); | ||
| } | ||
| } | ||
| } catch (...) { | ||
| // ignore parse failures or missing files | ||
| } |
There was a problem hiding this comment.
The catch (...) block is too broad. It swallows all exceptions without logging, which can hide problems like malformed JSON files. It's better to catch specific exceptions (e.g., nlohmann::json::exception) and log the error for easier debugging.
} catch (const nlohmann::json::exception& e) {
qWarning() << "Failed to parse JSON from " << p.c_str() << ":" << e.what();
} catch (const std::exception& e) {
qWarning() << "Failed to read file " << p.c_str() << ":" << e.what();
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: managing configurations for ll-cli via a new config subcommand and loading extensions from these configuration files. The implementation is extensive and well-structured. The new config command provides a comprehensive set of tools for managing application, base, and global settings. The logic for loading and merging configurations, and for resolving extensions, seems correct and follows a reasonable priority order.
My main feedback is about improving the robustness of the JSON file handling by using more specific exception catches instead of broad catch (...) blocks. This will make the code safer and easier to debug.
| } catch (...) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
The catch (...) block is too broad. It can hide unexpected errors, making debugging more difficult. It's better to catch specific exceptions, like nlohmann::json::exception for JSON errors.
} catch (const nlohmann::json::exception&) {
return std::nullopt;
}| } catch (...) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Similar to the other comment, this catch (...) block is too broad. It can hide various errors during file operations (e.g., I/O errors, filesystem errors from rename). Catching const std::exception& would be more specific and safer.
} catch (const std::exception&) {
return false;
}| } catch (...) { | ||
| // ignore parse failures or missing files | ||
| } |
There was a problem hiding this comment.
The catch (...) block is too broad and silently ignores all errors, which can make debugging very hard. It's better to catch specific exceptions, like nlohmann::json::exception, and log a warning. The code already uses qWarning for logging in this file, so it would be consistent to use it here as well. This pattern of broad, silent catches is repeated in loadExtensionsFromBase (line 151) and the readIfExists lambda (line 178).
} catch (const nlohmann::json::exception &e) {
// ignore parse failures or missing files, but log a warning for easier debugging.
qWarning() << "Failed to parse JSON from" << p.c_str() << ":" << e.what();
}
guanzi008
force-pushed
the
patch-3
branch
2 times, most recently
from
55f8b10 to
0b7ea34
Compare
guanzi008
force-pushed
the
patch-3
branch
3 times, most recently
from
a4dad91 to
fd862ef
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive configuration system for ll-cli, enabling fine-grained control over sandbox permissions and runtime behavior through JSON configuration files. The implementation is extensive, adding new CLI commands, modifying the runtime context to apply these configurations, and updating documentation. The code is generally well-structured, with good use of helper functions to manage the new complexity. My review has identified a potential race condition related to a change in repository locking, some inconsistencies in CLI command scope validation, and opportunities to improve exception handling by being more specific. Overall, this is a significant and valuable feature addition.
|
|
||
| auto ret = lockRepo(); | ||
| if (!ret) { | ||
| return LINGLONG_ERR( | ||
| QStringLiteral("failed to lock repo, underlying data will not be removed: %1") | ||
| .arg(ret.error().message().c_str())); | ||
| } | ||
|
|
||
| auto unlock = utils::finally::finally([this] { | ||
| auto ret = unlockRepo(); | ||
| if (!ret) { | ||
| qCritical() << "failed to unlock repo:" << ret.error().message(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Removing the repository lock from isRefBusy shifts the responsibility of locking to the callers. However, the PackageManager::Uninstall D-Bus method, which is a caller of isRefBusy, does not appear to have been updated to acquire a lock. This could introduce a race condition when checking if a reference is busy and during the uninstallation process itself, potentially leading to data corruption. Please ensure that all operations modifying the repository are properly serialized with locks.
| sub->callback([&, scopeOpts, apps, allow]() { | ||
| auto [scope, appId, baseId] = resolveScopeOrThrow(*scopeOpts); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| handleConfigResult(false); | ||
| return; | ||
| } | ||
| if (allow) { | ||
| jsonAllowConfigAccess(*j, *apps); | ||
| } else { | ||
| if (!jsonDenyConfigAccess(*j, *apps)) { | ||
| throw CLI::RuntimeError(_("No entries were removed"), 1); | ||
| } | ||
| } | ||
| handleConfigResult(saveConfig(scope, appId, baseId, *j)); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The help text and description for allow-config-home and deny-config-home imply they should only operate on the global configuration (--global). However, the current implementation also allows --app and --base scopes. This could lead to user confusion or incorrect configurations. To ensure the command behaves as documented, please add validation within the callback to restrict its use to the global scope.
sub->callback([&, scopeOpts, apps, allow]() {
auto [scope, appId, baseId] = resolveScopeOrThrow(*scopeOpts);
if (scope != Scope::Global) {
throw CLI::ValidationError("scope", "allow-config-home and deny-config-home only support --global scope");
}
auto j = openConfig(scope, appId, baseId);
if (!j) {
handleConfigResult(false);
return;
}
if (allow) {
jsonAllowConfigAccess(*j, *apps);
} else {
if (!jsonDenyConfigAccess(*j, *apps)) {
throw CLI::RuntimeError(_("No entries were removed"), 1);
}
}
handleConfigResult(saveConfig(scope, appId, baseId, *j));
});| sub->callback([&, scopeOpts, apps, allow]() { | ||
| auto [scope, appId, baseId] = resolveScopeOrThrow(*scopeOpts); | ||
| auto j = openConfig(scope, appId, baseId); | ||
| if (!j) { | ||
| handleConfigResult(false); | ||
| return; | ||
| } | ||
| if (allow) { | ||
| jsonAllowHostRoot(*j, *apps); | ||
| } else { | ||
| if (!jsonDenyHostRoot(*j, *apps)) { | ||
| throw CLI::RuntimeError(_("No entries were removed"), 1); | ||
| } | ||
| } | ||
| handleConfigResult(saveConfig(scope, appId, baseId, *j)); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Similar to allow-config-home, the allow-host-root and deny-host-root commands seem intended for global scope only, but the implementation doesn't enforce this. This could be misleading for users. Please add a check to ensure only Scope::Global is used for these commands.
sub->callback([&, scopeOpts, apps, allow]() {
auto [scope, appId, baseId] = resolveScopeOrThrow(*scopeOpts);
if (scope != Scope::Global) {
throw CLI::ValidationError("scope", "allow-host-root and deny-host-root only support --global scope");
}
auto j = openConfig(scope, appId, baseId);
if (!j) {
handleConfigResult(false);
return;
}
if (allow) {
jsonAllowHostRoot(*j, *apps);
} else {
if (!jsonDenyHostRoot(*j, *apps)) {
throw CLI::RuntimeError(_("No entries were removed"), 1);
}
}
handleConfigResult(saveConfig(scope, appId, baseId, *j));
});| } catch (...) { | ||
| // ignore parse failures or missing files | ||
| } |
There was a problem hiding this comment.
The catch (...) block is too broad and may hide important error information during debugging. It's better to catch more specific exceptions, such as nlohmann::json::parse_error or std::ifstream::failure. At a minimum, catching const std::exception& would allow you to log the error message, providing more context on failures. This applies to other similar try-catch blocks in this file.
} catch (const std::exception &e) {
qWarning() << "Failed to parse extensions from " << p.string().c_str() << ":" << e.what();
// ignore parse failures or missing files
}3 participants
Implement loading of extensions from config files
实现从配置文件加载扩展