Add missing Fanotify APIs from the libc crate into Nix. #2552

carlvoller · 2024-12-01T07:51:50Z

What does this PR do

Adds support for nix to receive additional Fanotify information records (such as libc::fanotify_event_info_fid, libc::fanotify_event_info_error and libc::fanotify_event_info_pidfd)
Adds abstractions over the new fanotify structs.
Adds new InitFlags to allow receiving these new information records.

This closes #2551

Checklist:

I have read CONTRIBUTING.md
I have written necessary tests and rustdoc comments
A change log has been added if this PR modifies nix's API

carlvoller · 2024-12-01T09:57:15Z

I'm not very sure whats going on with the solaris CI failure, but the test that is failing sys::test_uio::test_writev doesn't look related to the changes I've made.

Some guidance would be really appreciated 👍

SteveLauC · 2024-12-01T11:21:40Z

Hi @psumbera, could you please take a look at the Solaris CI failure? It failed in this line

nix/test/sys/test_uio.rs

Line 48 in 0f45593

let written = write_res.expect("couldn't write");

  ---- sys::test_uio::test_writev stdout ----
  thread 'sys::test_uio::test_writev' panicked at test/sys/test_uio.rs:48:29:
  couldn't write: EINVAL

psumbera · 2024-12-01T15:00:40Z

Hi @psumbera, could you please take a look at the Solaris CI failure? It failed in this line

nix/test/sys/test_uio.rs

Line 48 in 0f45593

let written = write_res.expect("couldn't write");
  ---- sys::test_uio::test_writev stdout ----
  thread 'sys::test_uio::test_writev' panicked at test/sys/test_uio.rs:48:29:
  couldn't write: EINVAL

I have seen this one. Seems that this one can fail randomly.

I will create PR to disable this test for Solaris. Sorry about that!

SteveLauC · 2024-12-01T15:05:34Z

Sorry about that!

No need to be sorry:)

I will create PR to disable this test for Solaris

Thanks, I will create an issue for this.

SteveLauC · 2024-12-02T00:45:57Z

I'm not very sure whats going on with the solaris CI failure, but the test that is failing sys::test_uio::test_writev doesn't look related to the changes I've made.

This test has been disabled, you can rebase your branch to fix the CI:)

carlvoller · 2024-12-03T11:24:12Z

Alright, just rebased 👍

…d armv7 issues

carlvoller · 2025-01-11T06:50:02Z

Hey any chance could this be merged soon? I would like to use these new features in another package I'm making.

SteveLauC · 2025-01-11T11:18:20Z

src/sys/fanotify.rs

+        /// Allows the receipt of events which contain additional information
+        /// about the underlying filesystem object correlated to an event.
+        ///
+        /// This will make `FanotifyEvent::fd` return `FAN_NOFD`.


Suggested change

/// This will make `FanotifyEvent::fd` return `FAN_NOFD`.

/// This will make [`FanotifyEvent::fd()`] return `None`.

SteveLauC · 2025-01-11T11:31:28Z

src/sys/fanotify.rs

+                current_event_offset += header.len as usize;
+            }
+
+            // libc::fanotify_event_info_header


An unused comment

SteveLauC · 2025-01-11T11:36:15Z

src/sys/fanotify.rs

+// Other fanotify_event_info records are not implemented as they don't exist in
+// the libc crate yet.


Emm, do we have any other types of information records? All the information record types mentioned in the man page are listed here, though the Linux kernel could add more in the future, which means this enum should be #[non_exhaustive]

SteveLauC · 2025-01-11T11:37:09Z

src/sys/fanotify.rs

+        /// Allows the receipt of events which contain additional information
+        /// about the underlying filesystem object correlated to an event.
+        ///
+        /// This will make `FanotifyEvent::fd` return `FAN_NOFD`.


Same here:

Suggested change

/// This will make `FanotifyEvent::fd` return `FAN_NOFD`.

/// This will make [`FanotifyEvent::fd()`] return `None`.

SteveLauC · 2025-01-11T11:39:08Z

src/sys/fanotify.rs

+/// information record received via [`Fanotify::read_events_with_info_records`].
+#[derive(Debug, Eq, Hash, PartialEq)]
+#[repr(transparent)]
+#[allow(missing_copy_implementations)]


Why is this not Copy?

SteveLauC · 2025-01-11T11:43:40Z

src/sys/fanotify.rs

+                    // This isn't found in the fanotify(7) documentation, but the fanotify_init(2) documentation
+                    // https://man7.org/linux/man-pages/man2/fanotify_init.2.html


You mean types FAN_EVENT_INFO_TYPE_NEW_DFID_NAME and FAN_EVENT_INFO_TYPE_OLD_DFID_NAME?

SteveLauC · 2025-01-11T11:52:49Z

src/sys/fanotify.rs

+
+                        Some(FanotifyInfoRecord::Fid(FanotifyFidRecord {
+                            record: LibcFanotifyFidRecord(record),
+                            handle_bytes: file_handle_ptr,


This looks like a dangling pointer as it comes from buffer, which belongs to the current function frame

SteveLauC · 2025-01-11T11:53:50Z

src/sys/fanotify.rs

+                    .get_struct::<libc::fanotify_event_info_header>(
+                        &buffer,
+                        current_event_offset,
+                    );


If possible, I hope we can inspect the header without doing any copy

SteveLauC · 2025-01-11T12:03:29Z

src/sys/fanotify.rs

+    /// The specific info_type for this Fid Record. Fanotify can return an Fid Record
+    /// with many different possible info_types. The info_type is not always necessary
+    /// but can be useful for connecting similar events together (like a FAN_RENAME) 
+    pub fn info_type(&self) -> u8 {


I think it would be nice to have an enum for the info type:

libc_enum! { #[repr(u8)] #[non_exhaustive] enum FanEventInfoType { FAN_EVENT_INFO_TYPE_FID, FAN_EVENT_INFO_TYPE_DFID, FAN_EVENT_INFO_TYPE_DFID_NAME, FAN_EVENT_INFO_TYPE_OLD_DFID_NAME, FAN_EVENT_INFO_TYPE_NEW_DFID_NAME, } }

SteveLauC · 2025-01-11T12:09:34Z

Hey any chance could this be merged soon? I would like to use these new features in another package I'm making.

Sorry for the late review, my non-working hours are quite limited due to my current full-time job, so I have been struggling to balance the few things I need to complete during that time.

Just did a round of review, I think tomorrow I will take a closer look

…now reads instead of copys, file_handle no longer returns pointer but returns [u8; 128]

carlvoller · 2025-01-15T18:26:23Z

Hi Steve,

I really appreciate you taking the time to review the code. I've just made a number of changes that should fix all of your concerns and comments above.

SteveLauC self-requested a review December 1, 2024 11:15

SteveLauC mentioned this pull request Dec 1, 2024

Test sys::test_uio::test_writev randomly fails on Solaris #2553

Open

carlvoller force-pushed the master branch from c8ec349 to 665c6ae Compare December 3, 2024 10:31

carlvoller added 8 commits January 8, 2025 17:50

Added information record support to fanotify

126db91

Added FanotifyFidRecord, FanotifyErrorRecord, FanotifyPidfdRecord

fcca358

Added additional match cases for all FAN_EVENT_INFO_TYPE_NAME info_types

89e861e

Added changelog file

e345f93

Fixed formatting issues, added conditional compilation to fix musl an…

28d754c

…d armv7 issues

Fixed clippy warning

2a4048a

Fixed event length miscalculation causing infinite loop

52a8dc1

Updated fanotify.rs to store file_handle separately

26d6d34

carlvoller force-pushed the master branch from 4514dc5 to 26d6d34 Compare January 8, 2025 09:50

Removed false clippy cast_ptr_alignment issue

077234d

SteveLauC reviewed Jan 11, 2025

View reviewed changes

carlvoller added 3 commits January 15, 2025 23:03

Added enum for FanotifyFidRecord event types, fixed comments, header …

380e319

…now reads instead of copys, file_handle no longer returns pointer but returns [u8; 128]

Added support for retrieving the appended name on an Fid Record

8be3098

Fixed clippy errors

39b2fc0

Added Copy and Clone to structs that can have it

a8fab20

carlvoller force-pushed the master branch from 21c54ec to a8fab20 Compare January 19, 2025 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing Fanotify APIs from the libc crate into Nix. #2552

Add missing Fanotify APIs from the libc crate into Nix. #2552

carlvoller commented Dec 1, 2024 •

edited

Loading

carlvoller commented Dec 1, 2024

SteveLauC commented Dec 1, 2024

psumbera commented Dec 1, 2024

SteveLauC commented Dec 1, 2024

SteveLauC commented Dec 2, 2024

carlvoller commented Dec 3, 2024

carlvoller commented Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC Jan 11, 2025

SteveLauC commented Jan 11, 2025

carlvoller commented Jan 15, 2025

	/// This will make `FanotifyEvent::fd` return `FAN_NOFD`.
	/// This will make [`FanotifyEvent::fd()`] return `None`.

		// Other fanotify_event_info records are not implemented as they don't exist in
		// the libc crate yet.

		// This isn't found in the fanotify(7) documentation, but the fanotify_init(2) documentation
		// https://man7.org/linux/man-pages/man2/fanotify_init.2.html

Add missing Fanotify APIs from the libc crate into Nix. #2552

Are you sure you want to change the base?

Add missing Fanotify APIs from the libc crate into Nix. #2552

Conversation

carlvoller commented Dec 1, 2024 • edited Loading

What does this PR do

Checklist:

carlvoller commented Dec 1, 2024

SteveLauC commented Dec 1, 2024

psumbera commented Dec 1, 2024

SteveLauC commented Dec 1, 2024

SteveLauC commented Dec 2, 2024

carlvoller commented Dec 3, 2024

carlvoller commented Jan 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC commented Jan 11, 2025

carlvoller commented Jan 15, 2025

carlvoller commented Dec 1, 2024 •

edited

Loading