Skip to content

Conversation

@ut003640
Copy link
Contributor

@ut003640 ut003640 commented Jan 29, 2026

used libcurl to check the connectivity

Log:replace the connectivity check type
Influence: 任务栏图标显示的状态是否正常
PMS: BUG-331389

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

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

Please try again later or upgrade to continue using Sourcery

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 29, 2026

TAG Bot

New tag: 2.0.80
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #481

@@ -0,0 +1,193 @@
// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

时间改下,其他文件的也改下

@ut003640 ut003640 force-pushed the master branch 2 times, most recently from 8dd918d to aad8e38 Compare January 30, 2026 01:32
@ut003640 ut003640 requested a review from caixr23 January 30, 2026 02:19
if (enableConnectivity) {
qCDebug(DSM) << "uses local connectivity checker";
LocalConnectionvityChecker *localChecker = new LocalConnectionvityChecker(m_ipConflictHandler, this);
qDebug() << "uses local connectivity checker";
Copy link
Contributor

Choose a reason for hiding this comment

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

使用qCDebug(DSM),日志都要分类

@deepin-ci-robot
Copy link

deepin pr auto review

这份代码主要对网络连接检测功能进行了重构,将原来基于QProcess调用curl命令的方式改为了直接使用libcurl库,并优化了检测逻辑。以下是对代码的详细审查和改进建议:

1. 语法逻辑

优点:

  • 代码结构清晰,将检测逻辑从LocalConnectionvityChecker中分离到StatusChecker类中,符合单一职责原则
  • 使用了Qt的信号槽机制进行线程间通信,避免了直接跨线程操作
  • 使用了防抖动机制(m_pendingCheckTimer)来避免频繁的网络检测

问题和建议:

  1. 线程安全问题

    • StatusChecker被移动到单独线程(m_thread)中,但HttpManagerget()方法创建的HttpReply对象是在调用线程中创建的,而不是在StatusChecker所在的线程中。这可能导致对象生命周期管理问题。
    • 建议:确保HttpReply对象在正确的线程中被创建和销毁,或者使用QObject::deleteLater()确保安全删除。
  2. 资源释放问题

    • HttpManager::get()方法返回的HttpReply对象由调用者负责删除,但代码中没有明确说明谁负责删除。在StatusChecker::realStartCheck()中创建了HttpReply但没有显式删除。
    • 建议:在HttpReply类中添加父子关系,或者使用智能指针管理生命周期。
  3. 配置值范围检查

    • 新增的配置项httpRequestTimeouthttpConnectTimeout没有进行范围检查,如果配置文件中设置了负数或过大的值,可能导致问题。
    • 建议:在SettingConfig中添加范围检查逻辑。

2. 代码质量

优点:

  • 代码注释清晰,特别是对新增功能的说明
  • 使用了C++11特性如nullptroverride
  • 错误处理较为完善,有日志输出

问题和建议:

  1. 魔法数字

    • 代码中存在一些魔法数字,如1000(1秒)、2000(2秒)、8(检测次数)等
    • 建议:将这些定义为常量,提高代码可读性和可维护性。
  2. 代码重复

    • setPortalUrl方法在StatusCheckerLocalConnectionvityChecker中都有实现,但逻辑略有不同
    • 建议:统一实现或提取公共逻辑
  3. 命名规范

    • LocalConnectionvityChecker类名拼写可能有误,应该是Connectivity而不是Connectionvity
    • 建议:修正类名拼写错误

3. 代码性能

优点:

  • 使用了libcurl直接进行HTTP请求,比通过QProcess调用curl命令效率更高
  • 实现了防抖动机制,减少了不必要的网络请求
  • 根据网络状态动态调整检测间隔,提高了效率

问题和建议:

  1. 同步阻塞问题

    • HttpManager::get()方法是同步阻塞的,在网络状况不好时可能会长时间阻塞
    • 建议:考虑使用异步方式实现,或者设置合理的超时时间
  2. 内存分配

    • HttpReply::setHeader方法中多次进行字符串操作和替换,可能产生大量临时对象
    • 建议:优化字符串处理逻辑,减少临时对象的创建
  3. 定时器管理

    • m_pendingCheckTimer使用单次触发模式,但每次调用startCheck()都会重新启动
    • 建议:考虑优化定时器管理逻辑,减少不必要的重启操作

4. 代码安全

优点:

  • 使用了CURLOPT_NOSIGNAL防止多线程死锁
  • 设置了合理的超时时间,防止长时间阻塞
  • 对网络请求结果进行了验证

问题和建议:

  1. URL验证

    • 没有对配置中的URL进行验证,如果配置了恶意URL可能导致安全问题
    • 建议:添加URL验证逻辑,确保只允许访问安全的URL
  2. 错误信息泄露

    • 直接将curl的错误信息记录到日志中,可能包含敏感信息
    • 建议:对错误信息进行过滤或脱敏处理
  3. 资源限制

    • 没有限制同时进行的网络请求数量,在极端情况下可能导致资源耗尽
    • 建议:添加请求队列和并发限制

5. 具体代码改进建议

  1. HttpManager::get()方法改进
HttpReply *HttpManager::get(const QString &url)
{
    // 添加URL验证
    if (!isValidUrl(url)) {
        qCWarning(DSM) << "Invalid URL:" << url;
        HttpReply *reply = new HttpReply(this);
        reply->setErrorMessage("Invalid URL");
        return reply;
    }
    
    // ... 现有代码 ...
    
    // 设置合理的超时范围
    long timeout = static_cast<long>(SettingConfig::instance()->httpRequestTimeout());
    timeout = qBound(5L, timeout, 300L); // 限制在5-300秒之间
    curl_easy_setopt(curl, CURLOPT_TIMEOUT, timeout);
    
    // ... 现有代码 ...
}
  1. StatusChecker::realStartCheck()方法改进
void StatusChecker::realStartCheck()
{
    // ... 现有代码 ...
    
    for (const QString &url : m_checkUrls) {
        HttpManager http;
        HttpReply *httpReply = http.get(url);
        
        // 确保HttpReply对象被正确删除
        QScopedPointer<HttpReply> replyGuard(httpReply);
        
        // ... 现有代码 ...
    }
    
    // ... 现有代码 ...
}
  1. 添加常量定义
// 在connectivitychecker.h中添加
namespace {
    constexpr int DEBOUNCE_INTERVAL_MS = 1000;  // 防抖动间隔
    constexpr int ACTIVE_CHECK_INTERVAL_MS = 2000;  // 活跃检测间隔
    constexpr int MAX_ACTIVE_CHECK_COUNT = 8;  // 最大活跃检测次数
}

总结

这次重构整体上提高了代码的质量和性能,将网络检测逻辑从基于QProcess的方式改为直接使用libcurl是一个很好的改进。主要需要关注的是线程安全、资源管理和错误处理等方面的问题。建议在后续版本中逐步完善这些方面,使代码更加健壮和安全。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ut003640

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

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.

3 participants