refactor: move VPN DNS mode handling to session plugin - #595
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 52cyb The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes the VPN DNS worker thread cleanup logic by removing a redundant deleteLater connection, tightening destructor cleanup flow, ensuring the worker is deleted from the correct thread, and nulling the worker pointer after cleanup to avoid crashes and use-after-free issues. Sequence diagram for updated VPN DNS worker thread cleanupsequenceDiagram
participant Controller as VpnDnsRouteController
participant WorkerThread as QThread
participant Worker as VpnDnsRouteWorker
Controller->>WorkerThread: start()
WorkerThread-->>Worker: finished (signal)
WorkerThread->>Worker: QObject::deleteLater()
Controller->>WorkerThread: ~VpnDnsRouteController()
Controller->>Worker: disconnect(this, nullptr, m_worker, nullptr)
alt m_workerThread && isRunning()
Controller->>WorkerThread: quit()
opt wait(3000) succeeds
Controller->>WorkerThread: wait(3000)
Controller->>Controller: m_worker = nullptr
end
else wait(3000) times out
Controller->>WorkerThread: terminate()
Controller->>WorkerThread: wait(500)
alt m_worker not null
Controller->>Worker: moveToThread(QThread::currentThread())
Controller->>Worker: delete m_worker
Controller->>Controller: m_worker = nullptr
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the destructor, consider guarding against a null m_workerThread (e.g., an early return as before) to avoid calling methods on a potentially uninitialized or already-cleared thread pointer.
- When moving the worker to the current thread in the termination path, it may be safer to check the worker’s existing thread affinity (worker->thread()) before calling moveToThread to avoid unnecessary moves or subtle lifecycle issues if the current thread is not intended to own the worker.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the destructor, consider guarding against a null m_workerThread (e.g., an early return as before) to avoid calling methods on a potentially uninitialized or already-cleared thread pointer.
- When moving the worker to the current thread in the termination path, it may be safer to check the worker’s existing thread affinity (worker->thread()) before calling moveToThread to avoid unnecessary moves or subtle lifecycle issues if the current thread is not intended to own the worker.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0443710 to
fe71a47
Compare
0b76656 to
c2418b0
Compare
|
@sourceAI review |
1. Relocate VPN DNS mode handler from libdde-network-core to network- service-plugin session service 2. Add VpnDnsModeHandler class to monitor VPN connections and trigger DNS mode application 3. Introduce VpnDnsModeWorker with async polkit authorization and queued request processing 4. Add VpnDnsModeApplyState and VpnDnsModeRequestQueue for state management 5. Move polkit policy file installation from libdde-network-core to network-service-plugin 6. Remove obsolete VpnDnsRouteController from core library and its dependencies 7. Add PolkitQt6-1 dependency to network-service-plugin CMake configuration Log: Refactored VPN DNS mode handling architecture, moving from core library to session plugin for better service separation Influence: 1. Verify VPN connection with preferred DNS mode applies DNS correctly 2. Verify VPN connection with secondary DNS mode applies DNS correctly 3. Verify VPN connection without dns-priority setting reverts link DNS properly 4. Test VPN connection updates while active to confirm DNS reconfiguration 5. Verify polkit authorization prompt appears when applying DNS settings 6. Test VPN disconnect and reconnect to ensure DNS state resets correctly 7. Verify concurrent VPN connections with different DNS modes work independently refactor: 将VPN DNS模式处理移动到会话插件 1. 将VPN DNS模式处理器从libdde-network-core迁移到network-service-plugin 会话服务 2. 新增VpnDnsModeHandler类用于监控VPN连接并触发DNS模式应用 3. 引入VpnDnsModeWorker,支持异步polkit授权和请求队列处理 4. 新增VpnDnsModeApplyState和VpnDnsModeRequestQueue用于状态管理 5. 将polkit策略文件安装从libdde-network-core迁移至network-service-plugin 6. 移除核心库中过时的VpnDnsRouteController及其依赖 7. 在network-service-plugin的CMake配置中添加PolkitQt6-1依赖 Log: 重构VPN DNS模式处理架构,从核心库移至会话插件以更好分离服务 Influence: 1. 验证首选DNS模式的VPN连接能正确应用DNS配置 2. 验证次要DNS模式的VPN连接能正确应用DNS配置 3. 验证未设置dns-priority的VPN连接能正确恢复链路DNS 4. 测试VPN连接激活状态下更新配置,确认DNS重新配置生效 5. 验证应用DNS设置时polkit授权提示是否正常弹出 6. 测试VPN断开和重连后DNS状态是否正确重置 7. 验证多个不同DNS模式的VPN连接能独立工作
deepin pr auto review★ 总体评分:75分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // network-service-plugin/src/session/vpndnsmode/vpndnsmodehandler.cpp
VpnDnsModeHandler::~VpnDnsModeHandler()
{
disconnect(this, nullptr, m_worker, nullptr);
disconnect(m_worker, nullptr, this, nullptr);
if (m_workerThread && m_workerThread->isRunning()) {
Q_ASSERT(QThread::currentThread() != m_workerThread);
if (m_worker) {
QMetaObject::invokeMethod(m_worker, "stop", Qt::BlockingQueuedConnection);
}
m_workerThread->quit();
m_workerThread->wait();
// 修复:线程事件循环退出后,deleteLater无法被处理,需手动释放
delete m_worker;
m_worker = nullptr;
}
if (m_workerThread) {
delete m_workerThread;
m_workerThread = nullptr;
}
} |
service-plugin session service
DNS mode application
request processing
management
network-service-plugin
dependencies
configuration
Log: Refactored VPN DNS mode handling architecture, moving from core
library to session plugin for better service separation
Influence:
properly
reconfiguration
correctly
independently
refactor: 将VPN DNS模式处理移动到会话插件
会话服务
Log: 重构VPN DNS模式处理架构,从核心库移至会话插件以更好分离服务
Influence: