fix: 修复usb手写屏热插拔不生效问题

[fly602]

usb手写屏接入时，会新增monitor，需要增加对该monitor的处理

Log: 修复usb手写屏热插拔不生效问题
PMS: BUG-276287
Influence: displaylink 显示问题

[sourcery-ai]

Sorry @fly602, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[deepin-ci-robot]

deepin pr auto review

Git Diff 代码审查报告

总体评价

这段代码主要修改了 xorg.go 文件中的 xMonitorManager 结构体相关的方法，主要涉及显示器管理逻辑的改进。代码整体质量良好，但存在一些并发安全和错误处理方面的问题。

详细审查意见

1. 并发安全问题

问题位置: doDiff() 方法中新增的代码

// 问题代码片段
if mm.hooks != nil {
    mm.mu.Unlock()
    mm.hooks.handleMonitorChanged(monitor)
    mm.mu.Lock()
}

问题描述:

• 在持有锁(mm.mu.Lock())的情况下调用外部回调函数(mm.hooks.handleMonitorChanged)是不安全的
• 虽然代码尝试在调用回调前解锁，但在回调完成后重新加锁，这种模式容易导致死锁
• 回调函数可能会尝试再次调用 xMonitorManager 的方法，导致重入问题

改进建议:

// 改进方案
if mm.hooks != nil {
    monitorCopy := *monitor // 创建副本，避免回调修改原始数据
    mm.mu.Unlock()
    mm.hooks.handleMonitorChanged(&monitorCopy)
    mm.mu.Lock()
}

2. 错误处理问题

问题位置: setMonitorFillMode() 方法

// 问题代码片段
if monitor == nil || !monitor.Enabled {
    logger.Warning("setMonitorFillMode monitor is nil, return.")
    return nil
}

问题描述:

• 当 monitor 为 nil 时，记录警告日志后直接返回 nil，可能掩盖实际问题
• 调用者无法区分是因未启用还是因 nil 指针而返回

改进建议:

// 改进方案
if monitor == nil {
    logger.Warning("setMonitorFillMode monitor is nil, return.")
    return fmt.Errorf("monitor is nil")
}
if !monitor.Enabled {
    logger.Debug("setMonitorFillMode monitor disabled, skip.")
    return nil
}

3. 内存安全问题

问题位置: compareAll() 方法

// 问题代码片段
if crtcInfo == nil {
    continue
}

问题描述:

• 虽然添加了 nil 检查，但需要确保 crtcInfo 为 nil 是预期行为
• 如果不是预期行为，应该记录日志或返回错误

改进建议:

// 改进方案
if crtcInfo == nil {
    logger.Warningf("crtc %v info is nil", crtc)
    continue
}

4. 资源管理问题

问题位置: handleOutputChanged() 方法

// 问题代码片段
if err != nil {
    logger.Warningf("get output %v info failed: %v", e.Output, err)
    delete(mm.outputs, e.Output)
    return
}

问题描述:

• 在获取输出信息失败时直接删除输出记录，可能导致状态不一致
• 没有考虑临时错误的情况，可能过早删除有效输出

改进建议:

// 改进方案
if err != nil {
    logger.Warningf("get output %v info failed: %v", e.Output, err)
    // 只在特定错误情况下删除输出，避免误删
    if isPermanentError(err) {
        delete(mm.outputs, e.Output)
    }
    return
}

5. 代码逻辑问题

问题位置: doDiff() 方法中新增的移除显示器逻辑

// 问题代码片段
for k, monitor := range oldMonitors {
    _, ok := newMap[k]
    if !ok {
        // 需要移除的monitor
        mm.mu.Unlock()
        logger.Info("remove monitor:", monitor.ID)
        mm.hooks.handleMonitorRemoved(monitor.ID)
        mm.mu.Lock()
    }
}

问题描述:

• 在持有锁的情况下解锁并调用外部回调，存在与前面相同的并发安全问题
• 没有检查 mm.hooks 是否为 nil 就直接调用

改进建议:

// 改进方案
for k, monitor := range oldMonitors {
    _, ok := newMap[k]
    if !ok {
        // 需要移除的monitor
        monitorID := monitor.ID // 保存ID，避免回调修改
        if mm.hooks != nil {
            mm.mu.Unlock()
            logger.Info("remove monitor:", monitorID)
            mm.hooks.handleMonitorRemoved(monitorID)
            mm.mu.Lock()
        }
    }
}

其他建议

1. 日志级别调整:

   • 将某些 logger.Info 改为 logger.Debug，避免日志过多
   • 例如: logger.Info("remove monitor:", monitor.ID) 可以改为 logger.Debug

2. 性能优化:

   • 考虑使用 sync.RWMutex 替代 sync.Mutex，如果读操作远多于写操作
   • 在 doDiff() 方法中，可以考虑减少锁的持有时间

3. 代码注释:

   • 为新增的复杂逻辑添加更详细的注释，解释为什么这样处理

总结

这段代码主要改进了显示器管理逻辑，添加了对新增和移除显示器的处理，但存在一些并发安全和错误处理方面的问题。建议重点关注并发安全问题，特别是在持有锁的情况下调用外部回调函数的场景。同时，改进错误处理逻辑，使错误信息更加明确，避免掩盖实际问题。

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fly602, mhduiy

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