fix: ensure error logs are output for OnlyLog type and unknown errors

[qiuzhiqian]

• Add indicator logging in CheckPkgSystemError when parsePkgSystemError returns error
• Return ErrorUnknown JobError instead of silently dropping error when parsePkgSystemError returns nil
• Move log output before JobId check in handleJobProgressInfo to ensure OnlyLog messages are not skipped

Bug: https://pms.uniontech.com/bug-view-352601.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

这段代码主要对 APT 包管理系统的错误处理逻辑和任务进度信息的处理流程进行了优化。下面我将从语法逻辑、代码质量、代码性能和代码安全四个方面进行详细审查并提出改进意见。

1. 语法逻辑审查

修改点分析：

• CheckPkgSystemError 函数：

  • 原逻辑：直接返回 parsePkgSystemError 的结果。
  • 新逻辑：增加了对 parsePkgSystemError 返回值的判断，并根据不同类型的错误（*system.JobError 或其他）记录不同的日志。如果 parsePkgSystemError 返回 nil，则创建一个新的 JobError 并记录日志。
  • 问题：逻辑分支较多，但整体清晰。需要注意的是，如果 parsePkgSystemError 返回 nil，新逻辑会创建一个 ErrorUnknown 类型的错误并返回，这改变了原函数的行为（原函数会返回 nil）。

• handleJobProgressInfo 函数：

  • 原逻辑：先查找 Job，再记录日志。
  • 新逻辑：先记录日志，再查找 Job。
  • 问题：新逻辑确保了即使 Job 不存在也能记录日志，这是合理的改进。

改进意见：

• 在 CheckPkgSystemError 中，如果 parsePkgSystemError 返回 nil，是否真的需要创建并返回一个 ErrorUnknown 类型的错误？这可能导致调用方误判。建议明确这种行为是否符合预期。

2. 代码质量审查

改进点：

• 日志记录的完整性：
  • 新逻辑确保了所有错误都会被记录，包括 parsePkgSystemError 无法解析的错误。
  • handleJobProgressInfo 中将日志记录提前，确保日志不会因为 Job 不存在而丢失。

改进意见：

• 代码重复：
  • CheckPkgSystemError 中有多处类似的 indicator 调用，可以提取为辅助函数以减少重复代码。
  • 示例：

    func logError(indicator system.Indicator, format string, args ...interface{}) {
        if indicator != nil {
            indicator(system.JobProgressInfo{
                OnlyLog:     true,
                OriginalLog: fmt.Sprintf(format, args...),
            })
        }
    }

• 错误处理的一致性：
  • CheckPkgSystemError 中对 parsePkgSystemError 返回 nil 的处理可能需要与调用方协商，确保行为一致。

3. 代码性能审查

潜在问题：

• 字符串拼接：

  • ErrDetail: outBuf.String() + errBuf.String()：直接拼接两个 bytes.Buffer 的内容，如果缓冲区较大，可能会影响性能。
  • 改进：可以使用 strings.Builder 或 fmt.Sprintf 优化：

    var detail strings.Builder
    detail.WriteString(outBuf.String())
    detail.WriteString(errBuf.String())
    ErrDetail: detail.String(),

• 日志记录的性能：

  • handleJobProgressInfo 中每次都会调用 time.Now().Format(time.DateTime)，如果日志频繁输出，可能会有性能开销。
  • 改进：如果日志时间戳的精度要求不高，可以考虑缓存时间或使用更轻量的时间格式。

4. 代码安全审查

潜在问题：

• 日志注入：

  • OriginalLog 直接使用 outBuf.String() 或 errBuf.String() 的内容，如果这些内容包含恶意数据（如控制字符），可能会导致日志注入问题。
  • 改进：对日志内容进行过滤或转义：

    import "strings"
    func sanitizeLog(s string) string {
        // 移除或转义危险字符
        return strings.Map(func(r rune) rune {
            if r < ' ' && r != '\n' && r != '\t' {
                return ' '
            }
            return r
        }, s)
    }

• 错误信息泄露：

  • ErrDetail 直接包含 outBuf 和 errBuf 的内容，可能泄露敏感信息（如系统路径、环境变量等）。
  • 改进：对错误信息进行脱敏处理，避免直接输出原始内容。

综合改进建议

以下是优化后的代码示例：

func CheckPkgSystemError(lock bool, indicator system.Indicator) error {
    if err == nil {
        return nil
    }
    if systemErr := parsePkgSystemError(outBuf.Bytes(), errBuf.Bytes()); systemErr != nil {
        if jobErr, ok := systemErr.(*system.JobError); ok {
            logError(indicator, "=== CheckPkg cmd failed: [%s] %s ===\n", jobErr.ErrType, jobErr.ErrDetail)
        } else {
            logError(indicator, "=== CheckPkg cmd failed: %s ===\n", errBuf.String())
        }
        return systemErr
    }

    // 如果 parsePkgSystemError 无法解析错误，创建一个通用错误
    var detail strings.Builder
    detail.WriteString(outBuf.String())
    detail.WriteString(errBuf.String())
    jobErr := &system.JobError{
        ErrType:   system.ErrorUnknown,
        ErrDetail: sanitizeLog(detail.String()),
    }
    logError(indicator, "=== CheckPkg cmd failed: [%s] %s ===\n", jobErr.ErrType, jobErr.ErrDetail)
    return jobErr
}

func logError(indicator system.Indicator, format string, args ...interface{}) {
    if indicator != nil {
        indicator(system.JobProgressInfo{
            OnlyLog:     true,
            OriginalLog: fmt.Sprintf(format, args...),
        })
    }
}

func sanitizeLog(s string) string {
    return strings.Map(func(r rune) rune {
        if r < ' ' && r != '\n' && r != '\t' {
            return ' '
        }
        return r
    }, s)
}

总结

1. 逻辑改进：新逻辑确保了所有错误都会被记录，但需要明确 parsePkgSystemError 返回 nil 时的行为是否符合预期。
2. 代码质量：通过提取辅助函数减少重复代码，提高可读性。
3. 性能优化：使用 strings.Builder 优化字符串拼接，减少不必要的性能开销。
4. 安全加固：对日志内容进行过滤和脱敏，防止日志注入和信息泄露。

这些改进将使代码更健壮、更安全，同时保持良好的可读性和可维护性。

[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