Skip to content

Conversation

@18202781743
Copy link
Contributor

@18202781743 18202781743 commented Feb 11, 2026

  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

Summary by Sourcery

Ensure notification DBus adaptor handles failed notification creation by logging instead of sending invalid DBus error messages.

Bug Fixes:

  • Stop sending standalone QDBusError messages without reply serials, aligning with DBus protocol requirements.

Enhancements:

  • Add warning log entries when notification creation fails to aid diagnosis of notification issues.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 11, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Replaces 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 fix

sequenceDiagram
    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
Loading

Flow diagram for Notify failure handling change

flowchart 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]
Loading

File-Level Changes

Change Details Files
Replace invalid standalone DBus error replies on Notify() failure with structured warning logging.
  • Include qlogging.h to enable qWarning logging with categories.
  • On Notify() failure (id == 0) in DbusAdaptor::Notify, remove creation and sending of QDBusError/QDBusMessage over QDBusConnection::sessionBus().
  • Log a categorized warning via qWarning(notifyLog) with the failing application name when notification creation fails in DbusAdaptor::Notify.
  • Apply the same replacement of DBus error sending with qWarning(notifyLog) logging in DDENotificationDbusAdaptor::Notify.
panels/notification/server/dbusadaptor.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@18202781743 18202781743 force-pushed the master branch 2 times, most recently from 8e8db83 to 8b6f088 Compare February 11, 2026 08:19
@deepin-ci-robot
Copy link

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

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

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-ci-robot
Copy link

deepin pr auto review

这段代码修改主要涉及版权年份的更新以及错误处理逻辑的变更。以下是对这段 diff 的详细审查和改进建议:

1. 语法与逻辑审查

修改内容分析:

  • 版权年份更新:从 2024 更新为 2024 - 2026。这是常规的版权维护操作,符合规范。
  • 错误处理逻辑变更
    • 原代码:当 id == 0 时,创建并发送了一个 QDBusError 类型的错误消息给 D-Bus 调用者。
    • 新代码:当 id == 0 时,仅使用 qWarning 打印日志,不再发送 D-Bus 错误消息。

潜在问题:

  1. 接口行为变更:原代码通过 D-Bus 返回错误(Error),新代码改为返回 0(Success)并打印日志。这违反了 D-Bus 接口规范,因为调用者无法通过返回值区分是通知被关闭还是通知发送失败。
  2. 返回值语义:根据 freedesktop.org 的通知规范,0 表示刷新现有通知失败,但新代码将其作为正常返回值,可能导致调用者误解。

2. 代码质量

优点:

  • 移除了冗余的 QDBusErrorQDBusMessage 对象创建,简化了代码。
  • 使用 qWarning 记录错误日志,便于调试。

缺点:

  • 错误处理逻辑不完整,调用者无法感知通知发送失败的情况。
  • 缺少对 manager()->Notify() 失败原因的详细记录(如参数校验、资源不足等)。

3. 性能

  • 性能影响:新代码减少了 D-Bus 错误消息的构造和发送,理论上性能略有提升,但影响极小(通常错误路径不常触发)。
  • 日志开销qWarning 的性能开销通常可以忽略,但在高频错误场景下可能需要考虑日志级别控制。

4. 安全性

  • 安全性影响:原代码通过 D-Bus 返回错误信息可能暴露内部实现细节,新代码仅记录日志,减少了信息泄露风险。
  • 日志安全appName 直接拼接到日志中,需确保其不包含敏感信息(如密码、路径等),否则可能泄露隐私。

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 增强日志记录

记录更多上下文信息(如 replacesIdexpireTimeout)以便排查问题:

qWarning(notifyLog) << "Notify failed for app:" << appName 
                    << "replacesId:" << replacesId 
                    << "expireTimeout:" << expireTimeout;

5.3 参数校验

在调用 manager()->Notify() 前增加参数合法性检查:

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;
}

总结

当前修改简化了错误处理逻辑,但牺牲了接口的健壮性。建议根据实际需求选择:

  • 如果通知失败是预期行为(如用户关闭通知),则新代码可接受。
  • 如果通知失败是异常情况,应恢复 D-Bus 错误返回并增强日志记录。

@18202781743 18202781743 merged commit a9a8f03 into linuxdeepin:master Feb 11, 2026
9 of 11 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.

3 participants