fs/deepin_err_notify: Use Netlink commands instead of the sysctl control interface #1374
Conversation
control interface Change the enable/disable control for error notifications from the sysctl interface to the Generic Netlink command interface, and extend the attribute information for error notifications. Major changes: - Remove the sysctl interface (/proc/sys/fs/deepin-err-notify-enable) - Add new Netlink commands: ENABLE, DISABLE, GET_STATUS - Add new error notification attributes: inode, ppid, uid, gid, status, etc. - Implement command handler functions to support dynamic enabling/disabling of the notification feature - Add Netlink attribute policies and operation tables Signed-off-by: electricface <[email protected]>
Reviewer's GuideSwitches Deepin filesystem error notification runtime control from a sysctl-based boolean in /proc/sys/fs/deepin-err-notify-enable to a Generic Netlink command interface, while extending the Netlink attributes and wiring them into the existing notification path. Sequence diagram for Netlink-based enable/disable/status controlsequenceDiagram
actor UserSpaceTool
participant GenericNetlinkCore
participant DeepinErrNotifyModule
UserSpaceTool->>GenericNetlinkCore: NLMSG(DEEPIN_ERR_NOTIFY_CMD_ENABLE)
GenericNetlinkCore->>DeepinErrNotifyModule: deepin_err_notify_cmd_enable
DeepinErrNotifyModule->>DeepinErrNotifyModule: deepin_err_notify_enable = 1
DeepinErrNotifyModule-->>GenericNetlinkCore: return 0
GenericNetlinkCore-->>UserSpaceTool: NLMSG(ack)
UserSpaceTool->>GenericNetlinkCore: NLMSG(DEEPIN_ERR_NOTIFY_CMD_DISABLE)
GenericNetlinkCore->>DeepinErrNotifyModule: deepin_err_notify_cmd_disable
DeepinErrNotifyModule->>DeepinErrNotifyModule: deepin_err_notify_enable = 0
DeepinErrNotifyModule-->>GenericNetlinkCore: return 0
GenericNetlinkCore-->>UserSpaceTool: NLMSG(ack)
UserSpaceTool->>GenericNetlinkCore: NLMSG(DEEPIN_ERR_NOTIFY_CMD_GET_STATUS)
GenericNetlinkCore->>DeepinErrNotifyModule: deepin_err_notify_cmd_get_status
DeepinErrNotifyModule->>DeepinErrNotifyModule: nlmsg_new + genlmsg_put
DeepinErrNotifyModule->>DeepinErrNotifyModule: nla_put_u8(STATUS, deepin_err_notify_enable)
DeepinErrNotifyModule-->>GenericNetlinkCore: genlmsg_reply
GenericNetlinkCore-->>UserSpaceTool: NLMSG(STATUS=u8)
Flow diagram for read-only filesystem error notification via Netlinkflowchart LR
A[FilesystemError in Deepin filesystem] --> B[deepin_check_and_notify_ro_fs_err]
B --> C{deepin_err_notify_enabled}
C -- false --> D[Return without notification]
C -- true --> E[deepin_send_ro_fs_err_notification]
subgraph DeepinErrNotifyModule
E --> F[genlmsg_put with DEEPIN_ERR_NOTIFY_CMD_ERR_ROFS]
F --> G[nla_put attributes<br>- FILENAME<br>- INODE<br>- INODE_PARENT<br>- PID<br>- PPID<br>- COMM<br>- UID<br>- GID]
G --> H[genlmsg_multicast to MULTICAST_GROUP_ERR_EVENTS]
end
H --> I[UserSpaceListeners with Netlink sockets]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @electricface. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The
deepin_err_notify_enabled()function is now declaredinlinein the C file withoutstatic, which gives it external linkage and may create duplicate-definition issues at link time; consider either droppinginlineor marking itstatic inline(and moving it to a header if it’s meant to be inlined across translation units).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `deepin_err_notify_enabled()` function is now declared `inline` in the C file without `static`, which gives it external linkage and may create duplicate-definition issues at link time; consider either dropping `inline` or marking it `static inline` (and moving it to a header if it’s meant to be inlined across translation units).
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:74-77` </location>
<code_context>
static int deepin_err_notify_enable __read_mostly = 0;
-int deepin_err_notify_enabled(void)
+/* Forward declaration of Generic Netlink family */
+static struct genl_family deepin_err_notify_genl_family;
+
+inline int deepin_err_notify_enabled(void)
{
return deepin_err_notify_initialized && deepin_err_notify_enable;
</code_context>
<issue_to_address>
**issue (bug_risk):** The `static` forward declaration of `deepin_err_notify_genl_family` conflicts with the later definition and will cause a redefinition error.
`static struct genl_family deepin_err_notify_genl_family;` at file scope is already a full definition. The later `static struct genl_family deepin_err_notify_genl_family __ro_after_init = { ... };` becomes a second, conflicting definition and should break the build. If you only need a forward declaration for `genlmsg_put`, make it `extern struct genl_family deepin_err_notify_genl_family;` (preferably in a shared header), or move the helper functions below the actual definition so no forward declaration is needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opsiff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| DEEPIN_ERR_NOTIFY_ATTR_FUNC_NAME, /* Function Name */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_FILENAME = 1, /* Filename (string) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_INODE = 2, /* Inode (u64) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT = |
There was a problem hiding this comment.
INODE_IS_PARENT或者INODE_FROM_PARENT更合适吧
There was a problem hiding this comment.
Pull request overview
This pull request migrates the Deepin filesystem error notification control interface from sysctl to Generic Netlink commands. The change modernizes the API by replacing the /proc/sys/fs/deepin-err-notify-enable interface with netlink-based commands, enabling runtime control of error notifications through a more flexible and extensible mechanism. The PR also extends the notification attributes to include additional metadata such as inode numbers, parent process IDs, and user/group IDs.
Key Changes:
- Replaced sysctl interface with Generic Netlink commands (ENABLE, DISABLE, GET_STATUS)
- Extended error notification attributes to include inode, ppid, uid, gid, and status fields
- Renamed multicast group from "ro_fs_events" to "err_events" and command from NOTIFY to ERR_ROFS
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static int deepin_err_notify_cmd_enable(struct sk_buff *skb, | ||
| struct genl_info *info) | ||
| { | ||
| deepin_err_notify_enable = 1; |
There was a problem hiding this comment.
The direct assignment to deepin_err_notify_enable is not thread-safe. Multiple concurrent calls to enable/disable commands could result in race conditions. Consider using atomic operations (e.g., atomic_t with atomic_set()) or appropriate memory barriers to ensure visibility across CPUs, especially since this variable is marked __read_mostly and can be read from multiple contexts simultaneously.
| static int deepin_err_notify_cmd_disable(struct sk_buff *skb, | ||
| struct genl_info *info) | ||
| { | ||
| deepin_err_notify_enable = 0; |
There was a problem hiding this comment.
The direct assignment to deepin_err_notify_enable is not thread-safe. Multiple concurrent calls to enable/disable commands could result in race conditions. Consider using atomic operations (e.g., atomic_t with atomic_set()) or appropriate memory barriers to ensure visibility across CPUs, especially since this variable is marked __read_mostly and can be read from multiple contexts simultaneously.
| #define DEEPIN_ERR_NOTIFY_FAMILY_NAME "DEEPIN_ENOTIFY" | ||
|
|
||
| /* Multicast group name for error events (used in Netlink communication) */ | ||
| #define MULTICAST_GROUP_ERR_EVENTS "err_events" |
There was a problem hiding this comment.
The multicast group name has changed from "ro_fs_events" to "err_events", which is a breaking change for userspace consumers. This means existing userspace tools listening to "ro_fs_events" will no longer receive notifications. Consider whether this breaking API change is intentional, and if so, ensure it's properly documented and communicated.
| static const struct nla_policy | ||
| deepin_err_notify_genl_policy[DEEPIN_ERR_NOTIFY_ATTR_MAX + 1] = { | ||
| [DEEPIN_ERR_NOTIFY_ATTR_FILENAME] = { .type = NLA_STRING }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_INODE] = { .type = NLA_U64 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT] = { .type = NLA_U8 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_PID] = { .type = NLA_U32 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_PPID] = { .type = NLA_U32 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_COMM] = { .type = NLA_STRING }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_UID] = { .type = NLA_U32 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_GID] = { .type = NLA_U32 }, | ||
| [DEEPIN_ERR_NOTIFY_ATTR_STATUS] = { .type = NLA_U8 }, | ||
| }; |
There was a problem hiding this comment.
The netlink attribute policy is missing an entry for DEEPIN_ERR_NOTIFY_ATTR_FUNC_NAME (index 10), even though this attribute is defined in the enum and used in the code (line 309-310). This could lead to validation issues or unexpected behavior when this attribute is sent. Add the missing policy entry for completeness and proper validation.
| /* Forward declaration of Generic Netlink family */ | ||
| static struct genl_family deepin_err_notify_genl_family; | ||
|
|
||
| inline int deepin_err_notify_enabled(void) |
There was a problem hiding this comment.
Adding the inline keyword to a non-static function in a .c file is unnecessary and could be problematic. The compiler will ignore it since the function definition is not in a header file, and the linker needs a real symbol. Consider removing inline or making the function static inline if it should be optimized for inlining within this compilation unit only.
| DEEPIN_ERR_NOTIFY_ATTR_FILENAME = 1, /* Filename (string) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_INODE = 2, /* Inode (u64) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_INODE_PARENT = | ||
| 3, /* Bool: inode from parent (u8) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_PID = 4, /* Process ID (u32) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_PPID = 5, /* Parent Process ID (u32) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_COMM = | ||
| 6, /* Process Short Name (string, 15 chars max) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_UID = 7, /* User ID (u32) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_GID = 8, /* Group ID (u32) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_STATUS = 9, /* Enable/Disable Status (u8) */ | ||
| DEEPIN_ERR_NOTIFY_ATTR_FUNC_NAME = 10, /* Function Name (string) */ |
There was a problem hiding this comment.
The explicit numbering of enum values (1, 2, 3, etc.) is redundant since they follow sequential order. This adds maintenance overhead without providing value. Consider removing the explicit numbers and relying on automatic enum value assignment, which is the standard pattern in Linux kernel code, unless there's a specific ABI compatibility reason to maintain these exact values.
| DEEPIN_ERR_NOTIFY_CMD_ERR_ROFS = | ||
| 1, /* Read Only Filesystem Error Notify Command */ | ||
| DEEPIN_ERR_NOTIFY_CMD_ENABLE = 2, /* Enable error notification */ | ||
| DEEPIN_ERR_NOTIFY_CMD_DISABLE = 3, /* Disable error notification */ | ||
| DEEPIN_ERR_NOTIFY_CMD_GET_STATUS = 4, /* Get enable/disable status */ |
There was a problem hiding this comment.
The explicit numbering of enum values (1, 2, 3, 4) is redundant since they follow sequential order. This adds maintenance overhead without providing value. Consider removing the explicit numbers and relying on automatic enum value assignment, which is the standard pattern in Linux kernel code, unless there's a specific ABI compatibility reason to maintain these exact values.
3 participants
Change the enable/disable control for error notifications from
the sysctl interface to the Generic Netlink command interface,
and extend the attribute information for error notifications.
Major changes:
status, etc.
enabling/disabling of the notification feature
Signed-off-by: electricface [email protected]
Summary by Sourcery
Replace the Deepin filesystem error notification control path with a Generic Netlink–based interface and extend the notification metadata.
Enhancements: