Add missing Fanotify APIs from the libc crate into Nix. by carlvoller · Pull Request #2552 · nix-rust/nix

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 👍

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

+        /// 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

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


An unused comment

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

+// 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

+        /// 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

+/// 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

+                    // 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

+
+                        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

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

+    /// 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

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.

carlvoller · 2025-02-08T09:22:48Z

Hi Steve,

Any chance could you take another look at my changes above? Thanks!

…d armv7 issues

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

carlvoller · 2025-03-02T10:13:20Z

Hi Steve,

Bumping this again in case you missed this. I would really appreciate it if you could help me take a look at the latest changes whenever you're next available.

luochenglcs · 2025-04-03T08:48:26Z

What's the latest?

carlvoller · 2025-04-03T08:49:52Z

Hi @SteveLauC,

Just bumping the above again. It would really be great if this could be merged soon.

fzylogic · 2025-06-11T16:04:32Z

Is there anything the community can do to help get this merged?

carlvoller · 2025-06-13T06:31:04Z

Hi @SteveLauC,

Giving this another bump. It would be really good to know how we can move forward with getting this merged. Or is there another maintainer we can ping here that could help us with the review process?

Thanks!

joebonrichie · 2026-03-13T22:08:47Z

Having a nice wrapper in nix over open_by_handle_at() to resolve the FanotifyInfoRecord name/handle back into the full path would be nice in the future, but probably outside the scope of this PR.

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 force-pushed the master branch from 4514dc5 to 26d6d34 Compare January 8, 2025 09:50

SteveLauC reviewed Jan 11, 2025

View reviewed changes

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

carlvoller added 13 commits March 2, 2025 17:51

Added information record support to fanotify

c908c06

Added FanotifyFidRecord, FanotifyErrorRecord, FanotifyPidfdRecord

ece88b2

Added additional match cases for all FAN_EVENT_INFO_TYPE_NAME info_types

ea62c96

Added changelog file

3af1c4c

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

d10b82e

…d armv7 issues

Fixed clippy warning

0afe35e

Fixed event length miscalculation causing infinite loop

e99ab6b

Updated fanotify.rs to store file_handle separately

31414ce

Removed false clippy cast_ptr_alignment issue

57134c2

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

b283943

…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

aa3aac3

Fixed clippy errors

dd9b49c

Added Copy and Clone to structs that can have it

0e3124a

carlvoller force-pushed the master branch from a8fab20 to 0e3124a Compare March 2, 2025 09:56

	/// 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

Conversation

carlvoller commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do

Checklist:

Uh oh!

carlvoller commented Dec 1, 2024

Uh oh!

SteveLauC commented Dec 1, 2024

Uh oh!

psumbera commented Dec 1, 2024

Uh oh!

SteveLauC commented Dec 1, 2024

Uh oh!

SteveLauC commented Dec 2, 2024

Uh oh!

carlvoller commented Dec 3, 2024

Uh oh!

carlvoller commented Jan 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveLauC commented Jan 11, 2025

Uh oh!

carlvoller commented Jan 15, 2025

Uh oh!

carlvoller commented Feb 8, 2025

Uh oh!

carlvoller commented Mar 2, 2025

Uh oh!

luochenglcs commented Apr 3, 2025

Uh oh!

carlvoller commented Apr 3, 2025

Uh oh!

fzylogic commented Jun 11, 2025

Uh oh!

carlvoller commented Jun 13, 2025

Uh oh!

joebonrichie commented Mar 13, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carlvoller commented Dec 1, 2024 •

edited

Loading