fix: expand event log to all non-community editions / 修复事件日志启用条件为非社区版#1114
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the event logging enablement condition so that it is enabled for all editions except UosCommunity instead of only UosProfessional, by changing the edition comparison logic when parsing /etc/os-version and updating the associated comments. Flow diagram for event log enablement conditionflowchart TD
A[Read /etc/os-version] --> B{EditionName found?}
B -- No --> C[Return false]
B -- Yes --> D{EditionName == Community?}
D -- Yes --> E[Return false]
D -- No --> F[Return true]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Since the check now hinges on
edition != "Community", consider normalizing theEditionNamevalue (e.g., trimming whitespace and handling case) to avoid unexpected behavior from minor formatting differences in/etc/os-version. - The logic currently returns
truewhenEditionNameis missing or malformed; if the intent is to restrict logging to non-community editions only, you may want to explicitly handle the case where noEditionNameis found instead of implicitly enabling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the check now hinges on `edition != "Community"`, consider normalizing the `EditionName` value (e.g., trimming whitespace and handling case) to avoid unexpected behavior from minor formatting differences in `/etc/os-version`.
- The logic currently returns `true` when `EditionName` is missing or malformed; if the intent is to restrict logging to non-community editions only, you may want to explicitly handle the case where no `EditionName` is found instead of implicitly enabling.
## Individual Comments
### Comment 1
<location path="session/eventlog/event_sdk.cpp" line_range="32-35" />
<code_context>
return true;
#else
- // Production mode: only enable for UosProfessional edition
+ // Production mode: disable for UosCommunity edition
// Read /etc/os-version and check EditionName
ifstream osVersion("/etc/os-version");
</code_context>
<issue_to_address>
**issue:** The implementation does not fully match the "all non-Community editions enabled" intent when /etc/os-version is missing or lacks EditionName.
Given the intent to "disable only for Community", this logic still returns false when `/etc/os-version` is missing or lacks an `EditionName=` line, effectively disabling event logging in those cases. To align with the intent, consider returning `true` when the file or `EditionName` is absent, and only returning `false` when `EditionName=Community` is explicitly found.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d7fc262 to
d743e5d
Compare
deepin pr auto review你好!我是CodeGeeX。我已仔细审查了你提供的 下面我将从语法逻辑、代码质量、代码性能和代码安全四个维度为你进行详细的代码审查,并提出改进意见: 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进后的代码建议综合以上分析,我为你提供一版优化后的代码(假设项目支持 C++17,如果仅支持 C++11,可将 // Check if event logging should be enabled based on system edition
// Enabled for all editions except UosCommunity
// Reads /etc/os-version to determine edition
static bool shouldEnableEventLog()
{
#ifdef DEBUG
// Debug mode: enable for current system version
return true;
#else
// Production mode: disable for UosCommunity edition
// Read /etc/os-version and check EditionName
std::ifstream osVersion("/etc/os-version");
if (!osVersion.is_open()) {
return true; // 默认开启
}
std::string line;
const std::string prefix = "EditionName=";
while (std::getline(osVersion, line)) {
// 使用 starts_with (C++20) 或 find (C++11) 检查前缀
if (line.find(prefix) == 0) {
// 避免 substr 产生的字符串拷贝,直接在原字符串上比较
// 或者使用 C++17 的 std::string_view
std::string edition = line.substr(prefix.length());
// 安全性增强:可以在此处添加 trim 空格和转换为小写的逻辑
// trim(edition);
// to_lower(edition);
return edition != "Community";
}
}
return true; // 未找到 EditionName 字段,默认开启
#endif
}主要改进点说明:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233, mhduiy 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 |
PMS: BUG-362029
Summary by Sourcery
Bug Fixes: