Skip to content

Fix: [file-enumerator] use g_file_enumerator_next_file()#260

Merged
Johnson-zs merged 1 commit intolinuxdeepin:release/eaglefrom
GongHeng2017:202602252125-release-eagle-fix
Mar 3, 2026
Merged

Fix: [file-enumerator] use g_file_enumerator_next_file()#260
Johnson-zs merged 1 commit intolinuxdeepin:release/eaglefrom
GongHeng2017:202602252125-release-eagle-fix

Conversation

@GongHeng2017
Copy link
Contributor

--Replace use of g_file_enumerator_iterate() with g_file_enumerator_next_file()
in DEnumerator::hasNext() so the enumerator's internal cursor is advanced by
GIO and error/end conditions are handled unambiguously.

--Using g_file_enumerator_next_file() yields clearer semantics: non-NULL -> next entry,
NULL + NULL error -> end, NULL + non-NULL error -> error. This avoids premature termination
caused by backend-specific iterate() quirks.

--Construct next entry URL via the existing buildUrl(...) helper and preserve
existing logic that creates the DFileInfo from the returned GFileInfo. Preserve
existing error handling and d->checkFilter() behavior and recursive skipping of filtered entries.

--Fix URL construction so joining directory path and filename does not introduce
an extra '/' (previously produced e.g. .../vault_unlocked//file). The logic now
concatenates carefully (handles root path and trailing slashes) to avoid duplicate separators.

Log: fix issue
Bug: https://pms.uniontech.com//bug-view-351177.html

Copy link
Contributor

@Johnson-zs Johnson-zs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

提到master

@GongHeng2017 GongHeng2017 force-pushed the 202602252125-release-eagle-fix branch from 5f38fab to 5cbbb8b Compare March 2, 2026 09:20
    --Replace use of g_file_enumerator_iterate() with g_file_enumerator_next_file()
    in DEnumerator::hasNext() so the enumerator's internal cursor is advanced by
    GIO and error/end conditions are handled unambiguously.

    --Using g_file_enumerator_next_file() yields clearer semantics: non-NULL -> next entry,
    NULL + NULL error -> end, NULL + non-NULL error -> error. This avoids premature termination
    caused by backend-specific iterate() quirks.

    --Construct next entry URL via the existing buildUrl(...) helper and preserve
    existing logic that creates the DFileInfo from the returned GFileInfo. Preserve
    existing error handling and d->checkFilter() behavior and recursive skipping of filtered entries.

    --Fix URL construction so joining directory path and filename does not introduce
    an extra '/' (previously produced e.g. .../vault_unlocked//file). The logic now
    concatenates carefully (handles root path and trailing slashes) to avoid duplicate separators.

Log: fix issue
Bug: https://pms.uniontech.com//bug-view-351177.html
@GongHeng2017 GongHeng2017 force-pushed the 202602252125-release-eagle-fix branch from 5cbbb8b to 7c4dfa5 Compare March 2, 2026 09:26
@GongHeng2017 GongHeng2017 requested a review from Johnson-zs March 3, 2026 01:22
@GongHeng2017 GongHeng2017 reopened this Mar 3, 2026
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码主要对 DEnumeratorPrivate::buildUrlDEnumerator::hasNext 两个函数进行了重构。整体来看,代码逻辑变得更清晰,且修复了一些潜在的逻辑错误。以下是对语法逻辑、代码质量、代码性能和代码安全方面的详细审查及改进意见:

1. 语法逻辑

DEnumeratorPrivate::buildUrl 函数改进:

  • 改进点:原代码在拼接路径时,仅简单判断了 url.path() == "/" 的情况。新代码增加了 dirPath.endsWith('/') 的判断,这能更好地处理 url.path() 末尾已经包含斜杠的情况(例如某些网络挂载路径或用户手动构造的 URL),避免了 // 双斜杠的出现。
  • 审查意见:逻辑正确且健壮性更好。

DEnumerator::hasNext 函数改进:

  • 改进点
    1. g_file_enumerator_iterate 替换为 g_file_enumerator_next_file。这是一个很好的改动。iterate 函数在某些后端(如网络文件系统)的行为可能不一致,且它同时返回 GFileInfoGFile,而 next_file 仅返回 GFileInfo,语义更明确。
    2. 逻辑流更清晰:先获取信息 -> 构造 URL -> 创建 FileInfo -> 检查过滤器。
    3. 错误处理更明确:当 nextInfo 为空且 gerror 存在时,设置错误并返回 true(通常意味着迭代结束但有错误状态),而不是直接 return false,这允许调用者检查 lastError()
  • 潜在逻辑问题
    • if (gerror) 分支中,代码设置了 d->nextUrl = QUrl();d->dfileInfoNext.reset();,然后返回了 true。这意味着 hasNext() 返回 true,但 next() 获取到的却是空对象。这可能会导致上层调用者在未检查 error() 的情况下解引用空指针或访问无效数据。
    • 建议:通常遇到严重错误时,迭代器应该停止。如果设计是让上层捕获错误,返回 true 是可以的,但必须确保上层逻辑足够健壮。或者,考虑在发生错误时返回 false 表示迭代结束,并确保错误码已设置。

2. 代码质量

  • 可读性:新代码通过引入 nextInfo 变量和清晰的注释,使得代码意图比原来的三元运算符和嵌套 if 更容易理解。
  • 注释:增加了关于 g_file_enumerator_next_file 的注释,解释了替换原因,这非常好。
  • 资源管理
    • 使用了 g_autoptr(GError),这是 GLib 的最佳实践,自动管理内存,防止泄漏。
    • g_object_unref(nextInfo) 调用正确,确保了 GFileInfo 对象被释放。
  • 一致性
    • hasNext 中,新代码不再依赖 g_file_get_pathg_file_get_uri,而是统一使用 buildUrl 结合 g_file_info_get_name。这统一了 URL 构建的逻辑,减少了代码重复,降低了因不同路径构造方式导致的不一致风险。

3. 代码性能

  • buildUrl 函数:虽然增加了 endsWith 判断,但这只是字符串操作,开销极小,且避免了后续正则处理双斜杠或路径解析的开销,是值得的。
  • hasNext 函数
    • g_file_enumerator_next_file 相比 iterate 可能会减少一次内部对象的构造(GFile),略微减轻内存分配压力。
    • 去掉了 g_file_get_pathg_file_get_uri 的调用,转而使用 g_file_info_get_name。后者通常直接从 GFileInfo 结构体中读取字符串,而前者可能涉及 I/O 或字符串转换,性能会有提升。

4. 代码安全

  • 空指针检查
    • hasNext 中,nextInfo 被正确检查。
    • buildUrl 中,fileNameconst char*,虽然代码中未显式检查 fileName 是否为 nullptr,但 QString(fileName) 能够安全处理 nullptr(构造为空字符串),而 buildUrl 通常用于文件系统迭代,fileName 来自底层 API,通常非空。如果追求极致安全,可以在函数入口添加断言或检查。
  • 内存安全
    • 新代码移除了 GFile *gfile 的局部变量声明,减少了混淆。
    • g_file_info_dup(nextInfo) 传递给 createFileInfoByUri,确保了所有权清晰。
  • 潜在的安全隐患(旧代码残留)
    • 在旧代码的 else 分支中(处理 URI 的情况),有一处明显的 Bug:
      d->nextUrl.setUserInfo(QString::fromLatin1("originPath::") + QString::fromLatin1(path));
      这里使用了 path(在 if 块中定义且已释放),而在 else 块中应该使用 uri。这是一个严重的 Use-After-Free(释放后使用)或未定义行为(变量作用域错误)。
    • 审查结果新代码成功修复了这个严重的安全 Bug。新代码不再区分 path 和 uri 的处理逻辑,统一通过 buildUrl 处理,消除了该隐患。

总结与改进建议

代码整体质量很高,修复了旧版本中的逻辑漏洞和严重 Bug。

建议修改点:

针对 DEnumerator::hasNext 中的错误处理逻辑,建议微调以避免上层逻辑困惑:

    // nextInfo == NULL: either finished or an error occurred
    if (gerror) {
        d->setErrorFromGError(gerror);
        // 发生错误时,清空状态并停止迭代。
        // 返回 false 表示没有下一个元素了,调用者应检查 error()。
        d->nextUrl = QUrl();
        d->dfileInfoNext.reset();
        return false; 
    }

理由
如果发生 I/O 错误(如权限被拒绝、读取错误),继续返回 true 并让上层调用 next() 获取空对象是不符合直觉的。通常迭代器在遇到不可恢复错误时应返回 false(表示迭代结束),同时通过 error() 接口暴露错误原因。这样更符合标准迭代器的设计模式(如 Java Iterator 或 C++ STL 迭代器在出错时的行为)。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017, Johnson-zs, liyigang1

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

@Johnson-zs Johnson-zs merged commit d203d35 into linuxdeepin:release/eagle Mar 3, 2026
36 of 38 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.

4 participants