Fix delivery support

[qiuzhiqian]

Task: https://pms.uniontech.com/task-view-388365.html

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiuzhiqian

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

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

_{You can retrigger this bot by commenting recheck in this Pull Request}

[deepin-ci-robot]

deepin pr auto review

代码审查报告

总体概述

这段代码主要实现了软件源管理功能，特别是P2P/Delivery源的更新和同步功能。代码引入了新的功能来处理平台源与P2P源的同步，并添加了相应的测试用例。整体代码结构较为清晰，但存在一些可以改进的地方。

语法与逻辑问题

1. replaceRepoSchemeWithDelivery函数中的逻辑问题

   func replaceRepoSchemeWithDelivery(line string) string {
       // ...
       for i := 1; i < len(fields); i++ {
           if strings.HasPrefix(fields[i], "[") {
               continue
           }
           switch {
           case strings.HasPrefix(fields[i], "https://"):
               fields[i] = "delivery://" + strings.TrimSuffix(strings.TrimPrefix(fields[i], "https://"), "/")
               return strings.Join(fields, " ")
           case strings.HasPrefix(fields[i], "http://"):
               fields[i] = "delivery://" + strings.TrimSuffix(strings.TrimPrefix(fields[i], "http://"), "/")
               return strings.Join(fields, " ")
           case strings.HasPrefix(fields[i], "delivery://"):
               return line
           }
       }
       return line
   }

   问题：该函数在替换URL后立即返回，这可能导致一行中有多个URL时只处理第一个。此外，对于包含方括号选项的deb源行（如deb [arch=amd64] https://...），处理可能不完整。

   建议修改：

   func replaceRepoSchemeWithDelivery(line string) string {
       fields := strings.Fields(line)
       if len(fields) < 2 || fields[0] != "deb" {
           return line
       }
       
       modified := false
       for i := 1; i < len(fields); i++ {
           // 跳过方括号中的选项
           if strings.HasPrefix(fields[i], "[") {
               for j := i; j < len(fields); j++ {
                   if strings.HasSuffix(fields[j], "]") {
                       i = j
                       break
                   }
               }
               continue
           }
           
           switch {
           case strings.HasPrefix(fields[i], "https://"):
               fields[i] = "delivery://" + strings.TrimSuffix(strings.TrimPrefix(fields[i], "https://"), "/")
               modified = true
           case strings.HasPrefix(fields[i], "http://"):
               fields[i] = "delivery://" + strings.TrimSuffix(strings.TrimPrefix(fields[i], "http://"), "/")
               modified = true
           case strings.HasPrefix(fields[i], "delivery://"):
               // 已经是delivery协议，无需修改
               return line
           }
       }
       
       if modified {
           return strings.Join(fields, " ")
       }
       return line
   }

2. UpdateP2pDefaultSourceDir函数中的错误处理

   err = utils.MoveFile(p2pSource.Name(), filepath.Join(sourceDir, filepath.Base(p2pSource.Name())))
   if err != nil {
       logger.Warning(err)
   }
   RefreshSymlinksForSourceDir(sourceDir)
   return nil

   问题：在MoveFile失败时仅记录警告但继续执行，这可能导致后续操作基于不完整的状态。此外，函数在最后返回nil，即使MoveFile失败。

   建议修改：

   err = utils.MoveFile(p2pSource.Name(), filepath.Join(sourceDir, filepath.Base(p2pSource.Name())))
   if err != nil {
       logger.Warningf("Failed to move p2p source file: %v", err)
       return fmt.Errorf("failed to move p2p source file: %w", err)
   }
   
   if err := RefreshSymlinksForSourceDir(sourceDir); err != nil {
       logger.Warningf("Failed to refresh symlinks: %v", err)
       return fmt.Errorf("failed to refresh symlinks: %w", err)
   }
   
   return nil

代码质量问题

1. 全局变量的使用

   var (
       tempSourceDirMu   sync.RWMutex
       tempSourceDirPath string
   )

   问题：使用全局变量可能导致代码难以测试和维护，特别是在并发环境中。

   建议修改：考虑将这些变量封装到一个结构体中，并通过依赖注入的方式使用。

2. 重复代码

   // 在多处重复出现的日志记录模式
   logger.Warningf("RefreshSymlinksForSourceDir: failed to read tempDir %s: %v", tempDir, err)
   logger.Warningf("RefreshSymlinksForSourceDir: failed to read sourceDir %s: %v", sourceDir, err)

   建议：创建一个辅助函数来统一处理错误日志记录。

3. 函数职责不单一
   UpdateP2pDefaultSourceDir函数既处理文件内容的替换，又负责文件系统的操作，职责不够单一。

   建议修改：将文件内容处理和文件系统操作分离到不同的函数中。

代码性能问题

1. 文件操作效率

   for _, file := range files {
       var content []byte
       filePath := filepath.Join(sourceDir, file.Name())
       if utils.IsSymlink(filePath) {
           targetPath, err := os.Readlink(filePath)
           // ...
           content, err = ioutil.ReadFile(targetPath)
       } else {
           content, err = ioutil.ReadFile(filePath)
       }
       // ...
   }

   问题：在循环中逐个读取文件，可能导致性能问题，特别是当文件数量较多或文件较大时。

   建议修改：考虑使用goroutine并行读取文件，但要注意控制并发数量。

2. 字符串处理效率

   var lines []string
   for _, line := range strings.Split(content, "\n") {
       // ...
       lines = append(lines, line)
   }
   return strings.Join(lines, "\n")

   问题：对于大文件，这种逐行处理方式可能导致内存使用效率不高。

   建议修改：考虑使用strings.Builder来构建结果字符串，或者使用bufio.Scanner逐行处理。

代码安全问题

1. 路径遍历风险

   targetPath, err := os.Readlink(linkPath)
   if err != nil {
       logger.Warningf("RefreshSymlinksForSourceDir: failed to read link %s: %v", linkPath, err)
       continue
   }
   
   if strings.HasPrefix(targetPath, sourceDir) {
       fileName := filepath.Base(targetPath)
       newTargetPath := filepath.Join(sourceDir, fileName)
       // ...
   }

   问题：虽然检查了targetPath是否以sourceDir开头，但没有验证targetPath是否在sourceDir的子目录中，可能存在路径遍历风险。

   建议修改：

   targetPath, err := os.Readlink(linkPath)
   if err != nil {
       logger.Warningf("RefreshSymlinksForSourceDir: failed to read link %s: %v", linkPath, err)
       continue
   }
   
   // 使用filepath.Rel确保targetPath在sourceDir的子目录中
   relPath, err := filepath.Rel(sourceDir, targetPath)
   if err != nil || strings.HasPrefix(relPath, "..") {
       logger.Warningf("RefreshSymlinksForSourceDir: invalid symlink target %s", targetPath)
       continue
   }
   
   fileName := filepath.Base(targetPath)
   newTargetPath := filepath.Join(sourceDir, fileName)
   // ...

2. 临时文件权限

   p2pSource, err := ioutil.TempFile("/tmp", "p2pSource-*.list")
   if err != nil {
       return fmt.Errorf("create temp file for p2p source failed: %v", err)
   }

   问题：临时文件创建后没有设置适当的权限，可能存在安全风险。

   建议修改：

   p2pSource, err := ioutil.TempFile("/tmp", "p2pSource-*.list")
   if err != nil {
       return fmt.Errorf("create temp file for p2p source failed: %v", err)
   }
   
   // 设置临时文件权限为0644
   if err := os.Chmod(p2pSource.Name(), 0644); err != nil {
       p2pSource.Close()
       os.Remove(p2pSource.Name())
       return fmt.Errorf("failed to set permissions for temp file: %v", err)
   }

3. 资源清理

   p2pSource, err := ioutil.TempFile("/tmp", "p2pSource-*.list")
   if err != nil {
       return fmt.Errorf("create temp file for p2p source failed: %v", err)
   }
   defer os.Remove(p2pSource.Name())

   问题：虽然使用了defer删除临时文件，但在某些错误情况下，文件可能没有被正确关闭。

   建议修改：

   p2pSource, err := ioutil.TempFile("/tmp", "p2pSource-*.list")
   if err != nil {
       return fmt.Errorf("create temp file for p2p source failed: %v", err)
   }
   
   // 确保文件在函数结束时关闭
   defer func() {
       p2pSource.Close()
       os.Remove(p2pSource.Name())
   }()

其他建议

1. 测试覆盖
   建议增加以下测试用例：

   • 测试RefreshSymlinksForSourceDir函数的各种边界情况
   • 测试UpdateP2pDefaultSourceDir函数在错误情况下的行为
   • 测试并发调用SetTempSourceDir和ClearTempSourceDir的情况

2. 文档注释
   建议为导出的函数添加更详细的文档注释，说明函数的用途、参数和返回值的含义。

3. 错误处理
   建议统一错误处理方式，例如使用fmt.Errorf包装错误，以便在调用栈中保留错误上下文。

总结

这段代码实现了重要的功能，但在错误处理、代码结构和安全性方面还有一些改进空间。建议按照上述意见进行修改，以提高代码的健壮性、可维护性和安全性。特别是要注意资源清理、路径验证和并发安全等问题。