Skip to content

Fix: [ops-log] The VGA log show error. - #409

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:release/eaglefrom
GongHeng2017:202509241107-eagle-fix
Sep 24, 2025
Merged

deepin-bot[bot] merged 1 commit into
linuxdeepin:release/eaglefrom
GongHeng2017:202509241107-eagle-fix

Conversation

@GongHeng2017

Copy link
Copy Markdown
Contributor

-- Change the get VAG log way.

Log: fix issue
Bug: https://pms.uniontech.com/bug-view-335611.html

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

根据提供的代码差异,我来对代码进行审查和分析:

1. 代码重构和架构改进

优点:

  • 代码重构将日志导出功能从多个分散的地方集中到了 OpsLogExport 类中,提高了代码的可维护性和可读性。
  • 移除了重复的代码,比如 exportOpsLog 和 exportHwOpsLog 的功能被整合到了新的 OpsLogExport 类中。

改进建议:

  • OpsLogExport 类中的方法过多,可以考虑进一步按功能模块拆分成多个类,比如 AppLogExporter、SystemLogExporter 等。
  • 类中的方法都是私有的,可以考虑将一些公共的功能提取出来作为独立的工具类。

2. 代码质量

优点:

  • 代码结构清晰,每个方法都有明确的职责。
  • 使用了现代 C++ 的特性,如 std::string 和 vector。

改进建议:

  • 大量使用了 system() 函数来执行 shell 命令,这种方式不够安全,且难以进行错误处理。建议使用 Qt 提供的进程管理类,如 QProcess。
  • 在 exportAppLogs() 方法中,有大量重复的代码,可以考虑使用配置文件或数据驱动的方式来减少重复代码。
  • 缺少错误处理机制,比如文件操作失败时的处理。

3. 代码性能

优点:

  • 使用了批量操作,比如 create_directories() 方法一次性创建多个目录。

改进建议:

  • 频繁调用 system() 函数会影响性能,可以考虑将多个命令合并成一个执行。
  • 在 copy_file_or_dir() 方法中,每次都要检查文件是否存在,可以考虑批量检查。

4. 代码安全

优点:

  • 使用了 2>/dev/null 来重定向错误输出,避免在日志中显示错误信息。

改进建议:

  • 使用 system() 函数执行 shell 命令存在安全风险,特别是当命令中包含用户输入时。建议使用 QProcess 并正确处理参数。
  • 在 execute_command() 方法中,直接拼接命令字符串可能会导致命令注入攻击。应该使用参数化的方式执行命令。
  • 文件权限设置过于宽松(chmod -R 777),应该根据实际需求设置更严格的权限。

5. 其他建议

  1. 添加日志记录功能,记录导出过程中的重要事件和错误。
  2. 考虑添加进度回调机制,以便在 UI 中显示导出进度。
  3. 对于长耗时操作,应该考虑添加取消功能。
  4. 添加单元测试,确保代码的正确性。
  5. 考虑使用异步方式执行导出操作,避免阻塞主线程。

总结:这次重构在代码组织和结构方面有明显的改进,但在安全性、性能和错误处理方面还有提升空间。建议逐步改进这些问题,以提高代码的整体质量。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017, max-lvs

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@GongHeng2017

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

This pr force merged! (status: unstable)

@deepin-bot
deepin-bot Bot merged commit 74be837 into linuxdeepin:release/eagle Sep 24, 2025
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants