fix: Prevent local rate-limiting config from overriding remote config.

[qiuzhiqian]

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

[github-actions]

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]

deepin pr auto review

Git Diff 代码审查

总体评价

这段代码主要实现了本地和远程限速配置的优先级处理逻辑，确保远程配置不会被本地配置覆盖。整体逻辑清晰，但存在一些可以改进的地方。

详细审查

1. 语法和逻辑问题

updater_ifc.go 中的 SetDownloadSpeedLimit 函数

func (u *Updater) SetDownloadSpeedLimit(limitConfig string) *dbus.Error {
	var limitConfigObj downloadSpeedLimitConfig
	if err := json.Unmarshal([]byte(limitConfig), &limitConfigObj); err != nil {
		logger.Warning(err)
		return dbusutil.ToError(err)
	}
	if u.downloadSpeedLimitConfigObj.IsOnlineSpeedLimit {
		// 在线限速优先级更高，如果通过dbus设置了一个本地限速，则不应该把在线限速的配置覆盖掉
		return nil
	}
	// dbus设置数据本地限速，强制将IsOnlineSpeedLimit设置为false
	limitConfigObj.IsOnlineSpeedLimit = false
	if err := u.setDownloadSpeedLimit(limitConfigObj); err != nil {
		logger.Warning(err)
		return dbusutil.ToError(err)
	}
	return nil
}

问题：

1. 在检查 u.downloadSpeedLimitConfigObj.IsOnlineSpeedLimit 时没有加锁，可能导致并发访问问题
2. 当在线限速优先级更高时直接返回 nil，但没有向调用者提供任何反馈信息，调用者可能不知道为什么设置失败

改进建议：

func (u *Updater) SetDownloadSpeedLimit(limitConfig string) *dbus.Error {
	var limitConfigObj downloadSpeedLimitConfig
	if err := json.Unmarshal([]byte(limitConfig), &limitConfigObj); err != nil {
		logger.Warning(err)
		return dbusutil.ToError(err)
	}
	
	u.PropsMu.Lock()
	defer u.PropsMu.Unlock()
	
	if u.downloadSpeedLimitConfigObj.IsOnlineSpeedLimit {
		// 在线限速优先级更高，如果通过dbus设置了一个本地限速，则不应该把在线限速的配置覆盖掉
		logger.Warning("Cannot set local speed limit while online speed limit is active")
		return dbusutil.ToError(fmt.Errorf("online speed limit is active, cannot override with local config"))
	}
	
	// dbus设置数据本地限速，强制将IsOnlineSpeedLimit设置为false
	limitConfigObj.IsOnlineSpeedLimit = false
	if err := u.setDownloadSpeedLimit(limitConfigObj); err != nil {
		logger.Warning(err)
		return dbusutil.ToError(err)
	}
	return nil
}

updater_ifc.go 中的 setDownloadSpeedLimit 函数

func (u *Updater) setDownloadSpeedLimit(limitConfigObj downloadSpeedLimitConfig) error {
	u.PropsMu.Lock()
	// ... 其他代码 ...
	u.PropsMu.Unlock()
	return nil
}

问题：

1. 函数内部已经有锁，但外部调用时可能再次加锁，导致死锁风险
2. 定时器回调函数中也获取了锁，增加了死锁的可能性

改进建议：

1. 将锁的使用统一到调用者或函数内部，避免嵌套加锁
2. 考虑使用 defer 确保锁一定会被释放
3. 将定时器回调中的锁获取移到最外层

2. 代码质量问题

日志记录不一致

在多个地方使用了不同的日志格式记录类似信息：

• logger.Infof("set download limit %v --> %v by platform", ...)
• logger.Infof("setPropDownloadSpeedLimitConfig: %s", ...)
• logger.Infof("set download limit %v --> %v", ...)

改进建议：统一日志格式，例如：

logger.Infof("DownloadSpeedLimit changed: %v --> %v (source: %s)", oldConfig, newConfig, source)

代码重复

manager_download.go 和 manager_update.go 中有相似的代码片段：

logger.Infof("set download limit %v --> %v by platform", ...)
if err := m.updater.setDownloadSpeedLimit(downloadSpeed); err != nil {
    logger.Warning(err)
}

改进建议：提取公共函数：

func (m *Manager) applyDownloadSpeedLimit(downloadSpeed downloadSpeedLimitConfig, source string) error {
    logger.Infof("set download limit %v --> %v by %s", m.updater.DownloadSpeedLimitConfig, downloadSpeed, source)
    return m.updater.setDownloadSpeedLimit(downloadSpeed)
}

3. 代码性能问题

1. JSON序列化/反序列化：频繁地进行JSON序列化和反序列化操作可能影响性能，特别是如果这些操作在热路径上。

   改进建议：考虑缓存已解析的配置对象，只在必要时进行序列化/反序列化。

2. 定时器使用：每次调用 setDownloadSpeedLimit 都会创建或重置一个1秒的定时器，如果频繁调用可能导致资源浪费。

   改进建议：考虑使用更智能的定时器策略，例如合并短时间内的多次更新。

4. 代码安全问题

1. 并发访问：多个地方访问共享变量 downloadSpeedLimitConfigObj 时没有一致的锁策略，可能导致数据竞争。

   改进建议：确保所有对共享变量的访问都在锁的保护下进行，可以考虑使用封装好的访问方法。

2. 错误处理：某些地方的错误处理不够完善，例如：

   if err := json.Unmarshal([]byte(limitConfig), &limitConfigObj); err != nil {
       logger.Warning(err)
       return dbusutil.ToError(err)
   }

   这里记录了警告但直接返回错误，可能不够清晰。

   改进建议：提供更详细的错误信息，帮助调试：

   if err := json.Unmarshal([]byte(limitConfig), &limitConfigObj); err != nil {
       logger.Warningf("Failed to unmarshal download speed limit config: %v, input: %s", err, limitConfig)
       return dbusutil.ToError(fmt.Errorf("invalid speed limit config: %v", err))
   }

3. 输入验证：没有对输入的配置参数进行充分验证，可能导致后续处理出现问题。

   改进建议：添加参数验证：

   func validateSpeedLimitConfig(config downloadSpeedLimitConfig) error {
       if config.MaxSpeed < 0 {
           return fmt.Errorf("invalid max speed: %d", config.MaxSpeed)
       }
       // 其他验证...
       return nil
   }

总结

这段代码实现了本地和远程限速配置的优先级处理，整体逻辑是正确的，但在并发安全、错误处理和代码组织方面有一些改进空间。建议：

1. 统一锁的使用策略，避免并发问题
2. 改进错误处理，提供更详细的错误信息
3. 统一日志格式，提高可读性
4. 提取公共代码，减少重复
5. 添加输入验证，提高代码健壮性
6. 考虑性能优化，特别是在热路径上的操作

这些改进将使代码更加健壮、可维护和安全。

[deepin-ci-robot]

[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:

• debian/deepin/OWNERS

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