fs/deepin_err_notify: Implement path combination function and improve read-only error notification for renaming #1381
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideImplements a new helper to construct full paths from a base path plus final component and wires it into Deepin’s filesystem error notification path for read-only rename failures, while slightly cleaning up attributes and logging behavior. Sequence diagram for read-only rename error notification flowsequenceDiagram
actor Process
participant VFS
participant DeepinErrNotify
participant UserspaceListener
Process->>VFS: vfs_rename(old_dentry, new_dentry)
VFS-->>VFS: Detect EROFS on target filesystem
VFS->>DeepinErrNotify: deepin_notify_rename_ro_fs_err(old_last, new_last, old_path, new_path)
DeepinErrNotify-->>DeepinErrNotify: Create deepin_path_last[2]
DeepinErrNotify->>DeepinErrNotify: prepare_and_notify_fs_error(path_lasts, 2)
DeepinErrNotify-->>UserspaceListener: Send filesystem error notification
UserspaceListener-->>UserspaceListener: Handle rename EROFS error event
DeepinErrNotify-->>VFS: Return from deepin_notify_rename_ro_fs_err
VFS-->>Process: Return -EROFS from rename
Class diagram for Deepin error notification helpersclassDiagram
class DeepinErrNotify {
+int deepin_err_notify_should_send()
+char* combine_path_and_last(char* buffer, const struct path* path, const char* last)
+void deepin_check_and_notify_ro_fs_err(const struct deepin_path_last* path_last, int count)
+void deepin_notify_rename_ro_fs_err(const struct qstr* old_last, const struct qstr* new_last, const struct path* old_path, const struct path* new_path)
+int prepare_and_notify_fs_error(const struct deepin_path_last* path_lasts, int count)
}
class deepin_path_last {
+struct path path
+const char* last
}
class qstr {
+const char* name
+unsigned int len
}
class path {
+struct vfsmount* mnt
+struct dentry* dentry
}
DeepinErrNotify ..> deepin_path_last : uses
DeepinErrNotify ..> qstr : uses
DeepinErrNotify ..> path : uses
DeepinErrNotify ..> d_absolute_path : calls
DeepinErrNotify ..> prepare_and_notify_fs_error : calls
DeepinErrNotify ..> pr_debug : logs
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. |
electricface
force-pushed
the
swt/err-notify-p5
branch
from
291cfa8 to
17cea1f
Compare
![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 - here's some feedback:
- In combine_path_and_last(), the memmove() call should copy base_len + 1 bytes to include the terminating NUL from d_absolute_path(), otherwise the resulting string can be left unterminated when base_path != buffer.
- deepin_notify_rename_ro_fs_err() passes old_last->name/new_last->name as C strings, but struct qstr::name is not guaranteed to be NUL-terminated; consider using qstr->len or copying into a temporary NUL-terminated buffer before passing to prepare_and_notify_fs_error().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In combine_path_and_last(), the memmove() call should copy base_len + 1 bytes to include the terminating NUL from d_absolute_path(), otherwise the resulting string can be left unterminated when base_path != buffer.
- deepin_notify_rename_ro_fs_err() passes old_last->name/new_last->name as C strings, but struct qstr::name is not guaranteed to be NUL-terminated; consider using qstr->len or copying into a temporary NUL-terminated buffer before passing to prepare_and_notify_fs_error().
## Individual Comments
### Comment 1
<location> `fs/deepin_err_notify.c:176-178` </location>
<code_context>
+ * not the start. We need the path at the beginning so we can
+ * append "/" + last to it.
+ */
+ if (base_path != buffer)
+ memmove(buffer, base_path, base_len);
+
+ /* Avoid appending an extra "/" after root directory "/" which would
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Copy the terminating null byte in memmove for better robustness.
`memmove(buffer, base_path, base_len);` omits the terminating `\0`. While current concatenation always overwrites the end of the base string so this is safe today, copying `base_len + 1` preserves the invariant that `buffer` is a valid C string immediately after the move and reduces risk from future refactors or early returns.
```c
if (base_path != buffer)
memmove(buffer, base_path, base_len + 1);
```
```suggestion
*/
if (base_path != buffer)
memmove(buffer, base_path, base_len + 1);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dongert
approved these changes
Dec 17, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dongert 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 |
improve read-only error notification for renaming - Implement the combine_path_and_last function to combine a base path and the final component to form a complete path. - Implement the deepin_notify_rename_ro_fs_err function, which calls prepare_and_notify_fs_error to send the error notification. Signed-off-by: electricface <[email protected]>
electricface
force-pushed
the
swt/err-notify-p5
branch
from
17cea1f to
da8cc55
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
path and the final component to form a complete path.
calls prepare_and_notify_fs_error to send the error notification.
Signed-off-by: electricface [email protected]
Summary by Sourcery
Implement path combination helper and extend read-only filesystem error notifications for rename operations.
New Features:
Enhancements: