Skip to content

feat: add config change callback for hardware-related settings#405

Merged
qiuzhiqian merged 1 commit intomasterfrom
fix-sync
Apr 30, 2026
Merged

feat: add config change callback for hardware-related settings#405
qiuzhiqian merged 1 commit intomasterfrom
fix-sync

Conversation

@qiuzhiqian
Copy link
Copy Markdown
Contributor

  • Export DSettingsKeyIncludeDiskInfo and DSettingsKeyGetHardwareIdByHelper
  • Add change callbacks for hardware-related config keys
  • Add syncHardwareRelatedData() to unify hardware data synchronization
  • Optimize token file writing to avoid redundant I/O
  • Use synchronous calls to ensure state consistency

Bug: https://pms.uniontech.com/bug-view-359599.html

- Export DSettingsKeyIncludeDiskInfo and DSettingsKeyGetHardwareIdByHelper
- Add change callbacks for hardware-related config keys
- Add syncHardwareRelatedData() to unify hardware data synchronization
- Optimize token file writing to avoid redundant I/O
- Use synchronous calls to ensure state consistency

Bug: https://pms.uniontech.com/bug-view-359599.html
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

xml seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码主要进行了三方面的修改:配置常量的导出、配置变更回调机制的完善,以及硬件相关数据同步逻辑的重构。以下是对这些修改的详细审查和改进建议:

1. 代码逻辑与语法审查

修改点分析:

  1. 常量导出:将 dSettingsKeyIncludeDiskInfodSettingsKeyGetHardwareIdByHelper 修改为首字母大写(DSettingsKey...),使其成为导出常量。
    • 意见:这是合理的。因为 initDSettingsChangedHandle 方法(在 manager.go 中)需要引用这些常量来注册回调,如果它们是私有的,跨包引用会受限或需要通过公共接口暴露。
  2. 配置变更回调增强:在 config.gogetConfigFromDSettings 函数中,针对 DSettingsKeyIncludeDiskInfoDSettingsKeyGetHardwareIdByHelper 的变更处理逻辑进行了增强。
    • 意见:原代码仅更新了内存中的配置值(c.IncludeDiskInfo)。新代码增加了对注册回调 dsettingsChangedCbMap 的触发,并传入了 oldValuenewValue。这使得外部(Manager)能够响应配置变化,逻辑更加完整。
  3. 数据同步逻辑重构
    • 新增了 Manager.syncHardwareRelatedData 方法,统一处理 Token 更新和 HardwareId 更新。
    • 移除了 manager_unit.go 中对 updateplatform 包的直接依赖,改为调用 Manager 的方法。
    • UpdateTokenConfigFile 中增加了文件内容比对,避免不必要的写入。
    • 意见:这是一个很好的重构。它将分散的逻辑集中到了 Manager 层,降低了耦合度,并且通过文件比对优化了 I/O 性能。

2. 代码质量审查

优点:

  • 关注点分离:将硬件数据同步逻辑封装在 syncHardwareRelatedData 中,使得 Manager 的职责更清晰。
  • 幂等性优化UpdateTokenConfigFile 中增加的 bytes.Equal 检查是一个很好的实践,避免了磁盘的无效写入和文件系统事件(如 inotify)的频繁触发。
  • 并发安全:新增了 hardwareDataMu 互斥锁,保护了 syncHardwareRelatedData 中的共享资源访问(如 updatePlatform.TokensetPropHardwareId),防止并发调用导致的数据竞争。

改进建议:

  1. 日志级别调整

    • systeminfo_utils.go 中,将 UpdateTokenConfigFile 的日志从 Debugf 改为 Infof
    • 建议:如果这个函数调用非常频繁(例如每次系统事件都触发),Infof 可能会产生大量日志。建议保留 Debugf 用于详细追踪,或者仅在值确实发生变化写入文件时使用 Infof,而在"内容未改变跳过写入"时使用 Debugf。目前的修改是全量 Info,可能会刷屏。
  2. 错误处理

    • UpdateTokenConfigFile 中:
      existingContent, err := os.ReadFile(tokenPath)
      if err == nil && bytes.Equal(existingContent, tokenContent) { ... }
      如果 os.ReadFile 失败(例如文件不存在),代码会继续执行 os.WriteFile。这是合理的。
      但是,如果 os.WriteFile 失败,仅记录了 logger.Warning(err),然后函数继续返回生成的 token 字符串。
      建议:检查调用方(syncHardwareRelatedData)对返回值的处理。如果写入失败但返回了 token,调用方可能会误以为文件已更新。建议在写入失败时返回空字符串或 error,并在 Manager 中处理该错误。
  3. 类型断言风险

    • config.go 的 switch case 中:
      newValue := v.Value().(bool)
      建议:虽然 DSettings 通常保证类型,但为了代码的健壮性,建议使用 comma-ok 模式进行类型断言,防止因配置源异常导致的 panic:
      newValue, ok := v.Value().(bool)
      if !ok {
          logger.Warningf("invalid type for %s, expected bool", key)
          return
      }

3. 代码性能审查

  • 文件 I/O 优化UpdateTokenConfigFile 中先读后写的策略是正确的。读取操作通常比写入快,且避免了相同内容的写入操作,减少了磁盘磨损和系统负载。
  • 锁的粒度hardwareDataMu 保护了整个同步过程。由于同步过程涉及文件 I/O(UpdateTokenConfigFile)和可能的耗时计算(GetHardwareIdByHelper),持有锁的时间可能较长。
    • 建议:如果 GetHardwareIdUpdateTokenConfigFile 非常耗时,且并发调用 syncHardwareRelatedData 的概率较高,可以考虑缩小锁的范围,仅保护对 m.updatePlatform.TokensetPropHardwareId 的赋值操作。但在当前场景下(配置变更触发),并发概率低,当前实现是可以接受的。

4. 代码安全审查

  • 文件权限:代码中保留了 0644 权限写入配置文件,并带有 #nosec G306 注释。
    • 意见0644 意味着所有用户可读,仅所有者可写。对于 Token 文件,如果包含敏感信息,通常建议更严格的权限(如 0600)。如果这是 APT 配置文件,需要被其他进程(如 apt-get)读取,则 0644 是必要的。请确认该 Token 文件的安全级别要求。如果确实需要 0644,保留 #nosec 是正确的。
  • 并发安全:如前所述,hardwareDataMu 的引入解决了潜在的并发问题。
  • DoS 风险UpdateTokenConfigFilesyncHardwareRelatedData 中被同步调用。如果该函数因为某种原因(如磁盘 I/O 阻塞)卡住,可能会导致 DSettings 的信号处理线程阻塞或 Manager 的处理延迟。
    • 建议:目前的代码在 initDSettingsChangedHandle 中直接调用 m.syncHardwareRelatedData。考虑到这是配置变更,不是高频热路径,同步调用通常是可以的。但如果担心阻塞,可以在 config.gocase 语句中已经使用了 go cb(...) 启动 goroutine,这意味着回调本身是异步的。但在 manager.go 的回调中直接调用了同步方法。建议确认是否需要在 manager.go 的回调中也使用 go 关键字,或者确认 syncHardwareRelatedData 的执行时间非常短。不过,考虑到它涉及文件 I/O,异步处理可能更稳妥。

总结

这段代码的整体质量较高,重构方向正确(解耦、集中管理、优化 I/O)。

主要改进建议:

  1. UpdateTokenConfigFile 中,针对写入失败的情况,考虑返回 error 而不是静默处理。
  2. config.go 的类型断言处增加安全检查。
  3. 评估 syncHardwareRelatedData 的执行耗时,如果耗时较长,建议在 Manager 的回调中使用 go m.syncHardwareRelatedData() 进行异步调用,以免阻塞配置监听循环。
  4. 检查 UpdateTokenConfigFile 的日志级别,避免高频操作产生过多日志。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiuzhiqian, zhaohuiw42

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

@qiuzhiqian qiuzhiqian merged commit cbf6724 into master Apr 30, 2026
24 of 29 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