Skip to content
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

inotify: support truncate and close calls #13869

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

michallenc
Copy link
Contributor

Summary

a1b14df

IN_MODIFY event should occur on file modification, which includes truncate. This is consistent with the inotify usage on Linux.

7f2858a

Close operation on file should lead to IN_CLOSE_WRITE or IN_CLOSE_NOWRITE notifications. This commits adds the notification support. Notifying on close is a little bit trickier as a lower layer may not have the full file path after successful close and inode release. Calling notification before close is not a solution since close might
not end successfully.
The solution is to obtain and buffer the path before calling close and then pass the buffered path to the notify_close. This required the change in notify_close function arguments: filep is no longer required, path and oflags are passed instead.

Impact

Inotify now reports notification on close and truncate operations.

Testing

Tested on SAMv7 custom board.

@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Oct 7, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 7, 2024

[Experimental Bot, please feedback here]

The PR does a good job of summarizing the changes and their impact. However, it's missing some key information required by the NuttX PR guidelines.

Here's a breakdown of what's missing and how to improve the PR:

Summary:

  • What functional part of the code is being changed? - The summary mentions IN_MODIFY event and notify_close function, but doesn't explicitly state that the changes are within the inotify subsystem. It should be clearly stated that these changes are related to the inotify functionality in NuttX.
  • Related NuttX Issue reference: - If this PR addresses a specific issue raised in the NuttX issue tracker, provide a link to that issue. Example: "Fixes arch/stackframe: fix heap buffer overflow #1234" or "Related to arch/stackframe: fix heap buffer overflow #1234".

Impact:

  • The impact section is too brief. While it correctly identifies that inotify now reports notifications on close and truncate operations, it needs more detail for each category:
    • Is new feature added? - Yes, support for IN_CLOSE_WRITE, IN_CLOSE_NOWRITE and truncate within IN_MODIFY are new features.
    • Impact on user: - YES. Users can now rely on inotify to receive notifications for file close and truncate operations. Describe any specific actions users might need to take to utilize these new events.
    • Impact on build: - Potentially YES if the changes require modifications to Kconfig options or build dependencies related to inotify. Specify any changes to the build system.
    • Impact on documentation: - YES. Documentation needs to be updated to describe the new events and their usage. Specify if the documentation updates are included in this PR or will be addressed separately.

Testing:

  • Build Host(s): Specify the operating system, CPU architecture, and compiler used for building NuttX.
  • Target(s): While you mention testing on a SAMv7 board, specify the board name and configuration.
  • Testing logs before change: This section is empty. Provide snippets of logs demonstrating the behavior before your changes.
  • Testing logs after change: Include log snippets demonstrating the correct behavior after your changes, specifically showing the new events being generated as expected.

To improve the PR:

  1. Expand the summary to explicitly mention the inotify subsystem and link any related NuttX issues.
  2. Provide more detail in the impact section for each category, specifying YES or NO and explaining the impact.
  3. Include complete build host and target information in the testing section.
  4. Add "before" and "after" testing logs that clearly demonstrate the changes in functionality.

fs/notify/inotify.c Outdated Show resolved Hide resolved
fs/notify/notify.h Outdated Show resolved Hide resolved
fs/vfs/fs_close.c Show resolved Hide resolved
@github-actions github-actions bot added the Area: Documentation Improvements or additions to documentation label Oct 7, 2024
IN_MODIFY event should occur on file modification, which includes
truncate. This is consistent with the inotify usage on Linux.

Signed-off-by: Michal Lenc <[email protected]>
Close operation on file should lead to IN_CLOSE_WRITE or
IN_CLOSE_NOWRITE notifications. This commits adds the notification
support. Notifying on close is a little bit trickier as a lower layer
may not have the full file path after successful close and inode release.
Calling notification before close is not a solution since close might
not end successfully.

The solution is to obtain and buffer the path before calling close
and then pass the buffered path to the notify_close. This required the
change in notify_close function arguments: filep is no longer
required, path and oflags are passed instead.

Signed-off-by: Michal Lenc <[email protected]>
IN_CLOSE_WRITE and IN_CLOSE_NOWRITE are now supported and IN_MODIFY
can be used for truncate as well.

Signed-off-by: Michal Lenc <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit dc6f406 into apache:master Oct 8, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Area: File System File System issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants