-
Notifications
You must be signed in to change notification settings - Fork 60
fix: remove invalid DBus error message sending #1447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces improper direct sending of standalone QDBusError messages on notification failures with internal logging via qWarning(), ensuring DBus protocol compliance while keeping notification behavior unchanged except for improved diagnostics. Sequence diagram for DBus Notify error handling after fixsequenceDiagram
actor DBusClient
participant DbusAdaptor
participant NotificationManager
participant DBusBus
participant SystemLog
DBusClient->>DBusBus: Send Notify call
DBusBus->>DbusAdaptor: Deliver Notify(appName, ...)
DbusAdaptor->>NotificationManager: Notify(appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout)
NotificationManager-->>DbusAdaptor: id (0 on failure, >0 on success)
alt Notify success
DbusAdaptor-->>DBusBus: Return id (>0) in method reply
DBusBus-->>DBusClient: Deliver successful reply
else Notify failure (id == 0)
DbusAdaptor->>SystemLog: qWarning notifyLog Notify failed for app: appName
DbusAdaptor-->>DBusBus: Return id 0 in method reply
DBusBus-->>DBusClient: Deliver reply with id 0
end
Flow diagram for Notify failure handling changeflowchart TD
A[Notify called on DbusAdaptor] --> B[Call manager Notify and get id]
B --> C{Is id == 0?}
C -- No --> D[Return id to caller]
C -- Yes --> E[Log warning with qWarning notifyLog]
E --> D[Return id 0 to caller]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8e8db83 to
8b6f088
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, yixinshark 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 |
1. Removed code that created and sent QDBusError messages directly via QDBusConnection::sessionBus().send() 2. Replaced with proper logging using qWarning() when notification creation fails 3. This fixes a protocol violation where error messages were being sent without proper reply serial numbers 4. The original implementation was creating standalone error messages which is not allowed by DBus specification 5. Now only logs the failure internally without attempting to send invalid DBus messages Influence: 1. Test notification creation with invalid parameters to ensure proper error handling 2. Verify that no invalid DBus messages are sent when notifications fail 3. Check system logs for warning messages when notification creation fails 4. Ensure notification service continues to function normally after failed requests 5. Test with various DBus clients to confirm protocol compliance fix: 移除无效的 DBus 错误消息发送 1. 删除了通过 QDBusConnection::sessionBus().send() 直接创建和发送 QDBusError 消息的代码 2. 替换为使用 qWarning() 进行适当日志记录,当通知创建失败时 3. 修复了协议违规问题,错误消息在没有正确回复序列号的情况下被发送 4. 原始实现创建了独立的错误消息,这违反了 DBus 规范 5. 现在仅在内部记录失败日志,而不尝试发送无效的 DBus 消息 Influence: 1. 测试使用无效参数创建通知,确保正确的错误处理 2. 验证当通知失败时不会发送无效的 DBus 消息 3. 检查系统日志中通知创建失败时的警告消息 4. 确保通知服务在请求失败后继续正常运行 5. 使用各种 DBus 客户端测试以确认协议合规性 PMS: BUG-350869
deepin pr auto review这段代码修改主要涉及版权年份的更新以及错误处理逻辑的变更。以下是对这段 diff 的详细审查和改进建议: 1. 语法与逻辑审查修改内容分析:
潜在问题:
2. 代码质量优点:
缺点:
3. 性能
4. 安全性
5. 改进建议5.1 恢复 D-Bus 错误返回(推荐)如果通知发送失败应被视为异常情况,建议保留 D-Bus 错误返回: uint id = manager()->Notify(appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout);
if (id == 0) {
qWarning(notifyLog) << "Notify failed for app:" << appName;
QDBusError error(QDBusError::Failed, "Notification failed to display");
sendErrorReply(error.name(), error.message());
return 0; // 明确返回失败值
}
return id;5.2 增强日志记录记录更多上下文信息(如 qWarning(notifyLog) << "Notify failed for app:" << appName
<< "replacesId:" << replacesId
<< "expireTimeout:" << expireTimeout;5.3 参数校验在调用 if (appName.isEmpty() || summary.isEmpty()) {
qWarning(notifyLog) << "Invalid parameters: appName or summary is empty";
sendErrorReply(QDBusError::InvalidArgs, "Empty appName or summary");
return 0;
}5.4 统一错误处理将错误处理逻辑封装为函数,避免重复代码: void DbusAdaptor::handleNotifyError(const QString &appName, uint replacesId) {
qWarning(notifyLog) << "Notify failed for app:" << appName << "replacesId:" << replacesId;
sendErrorReply(QDBusError::Failed, "Notification failed to display");
}
// 使用示例
if (id == 0) {
handleNotifyError(appName, replacesId);
return 0;
}总结当前修改简化了错误处理逻辑,但牺牲了接口的健壮性。建议根据实际需求选择:
|
QDBusConnection::sessionBus().send()
creation fails
without proper reply serial numbers
which is not allowed by DBus specification
invalid DBus messages
Influence:
error handling
fails
failed requests
fix: 移除无效的 DBus 错误消息发送
QDBusError 消息的代码
Influence:
PMS: BUG-350869
Summary by Sourcery
Ensure notification DBus adaptor handles failed notification creation by logging instead of sending invalid DBus error messages.
Bug Fixes:
Enhancements: