Add statx #1660

vi · 2022-02-09T21:22:56Z

Resolves #1649.

In current state this breaks the build for Musl targets.

src/fcntl.rs

asomers · 2022-03-06T15:54:43Z

src/fcntl/statx.rs

+);
+*/
+
+/// Attempt to retrieve stats of `pathname`. If `pathname` is relative, `dirfd` is used as starting directory for lookups. If `dirfs` is None, current directory is used.


Wrap to 80 cols, please.

src/fcntl/statx.rs

asomers · 2022-03-06T16:05:01Z

src/fcntl/statx.rs

+
+impl Statx {
+    /// Retrieve file type, if it has been returned by kernel
+    pub fn r#type(&self) -> Option<SFlag> {


This method name will be hard for users to type. I suggest filetype.

src/fcntl/statx.rs

asomers · 2022-03-06T16:08:38Z

src/fcntl/statx.rs

+    }
+
+    /// Retrieve file access time, if it has been returned by kernel
+    pub fn atime(&self) -> Option<libc::statx_timestamp> {


Returning the raw libc type isn't very useful. Is it identical to a struct timespec? If so, I suggest you convert it into a nix::sys::time::TimeSpec.

Done, also added the appropriate From TimeSpec implementation within the statx module. Or should it be moved to the place where other TimeSpec From implementations live with a duplicated #[cfg] line?

asomers · 2022-03-06T16:10:26Z

src/fcntl/statx.rs

+    }
+
+    /// Retrieve file size as a number of blocks, if it has been returned by kernel
+    pub fn blksize(&self) -> Option<u64> {


Is this identical to blocks?

No, probably I just forgot to fill in new doccomment and implementation after copying neighbouring method.

What is it supposed to do then? Because it's just returning the same stx_blocks variable.

Fixed to return blocksize, fixed the doccomment.

src/fcntl/statx.rs

Co-authored-by: Alan Somers <asomers@gmail.com>

src/fcntl/statx.rs

asomers · 2022-03-06T22:29:02Z

src/fcntl/statx.rs

+    }
+
+    /// Retrieve file size as a number of blocks, if it has been returned by kernel
+    pub fn blksize(&self) -> Option<u64> {


What is it supposed to do then? Because it's just returning the same stx_blocks variable.

src/fcntl/statx.rs

asomers · 2022-03-07T01:13:23Z

src/fcntl/statx.rs

+);
+
+/// Attempt to retrieve stats of `pathname`. If `pathname` is relative, `dirfd`
+/// is used as starting directory for lookups. If `dirfs` is None, current


Suggested change

/// is used as starting directory for lookups. If `dirfs` is None, current

/// is used as starting directory for lookups. If `dirfd` is None, current

asomers · 2022-03-07T01:15:21Z

src/fcntl/statx.rs

+/// is used as starting directory for lookups. If `dirfs` is None, current
+/// directory is used.
+/// `pathname` may be empty string. But instead of specifying empty string
+/// literal, you are adviced to use zero-terminated `CStr` to avoid extra allocation 


How about adding some examples to show usage? That would be especially helpful for the "empty pathname" case.

Good idea. Shall doc examples be marked as testable code (and assert something about typical external files e.g. /dev/null)?

asomers · 2022-03-07T01:16:35Z

src/fcntl/statx.rs

+pub struct Statx {
+    pub inner: libc::statx,
+}


Since it has only one member, it can be a NewType.

Suggested change

pub struct Statx {

pub inner: libc::statx,

}

#[repr(transparent)]

pub struct Statx(libc::statx);

Done (inner tuple field is still pub, unlike in the suggested change).

asomers · 2022-03-07T01:17:35Z

src/fcntl/statx.rs

+        }
+    }
+
+    /*


Why are these two methods commented out?

Because of current version of libc crate does not expose e.g. STATX_ATTR_VERITY. See my question at #1649 (comment).
Android target's libc lacks even more consts in libc.

I submitted a PR to add the missing constants on GNU/Linux

SteveLauC · 2022-10-04T23:40:49Z

Hey @vi :) What is the status of this PR? I really hope this can be merged

I just tried it myself, sadly, there is a lot of missing stuff in libc.

SteveLauC · 2022-10-05T00:08:46Z

src/fcntl.rs

@@ -889,3 +889,6 @@ pub fn posix_fallocate(fd: RawFd, offset: libc::off_t, len: libc::off_t) -> Resu
    }
 }
 }
+
+#[cfg(all(feature="fs", any(target_os = "android", target_os = "linux")))]


#[cfg(all( any( target_os = "android", all(target_os = "linux", not(target_env = "musl")), ), feature = "fs" ))]

Since this is not available on musl, this configuration should work

SteveLauC · 2022-10-05T03:09:16Z

src/fcntl/statx.rs

+
+    let mut dst = mem::MaybeUninit::uninit();
+    let res = pathname.with_nix_path(|cstr| unsafe {
+        libc::statx(


This wrapper function is only exposed on GNU/Linux as glibc added it.

What about making this statx module temporarily exclusive to GNU/Linux? When the missing stuff is added, we can easily add support for other platforms by changing a few lines of code.

vi · 2022-10-05T06:09:59Z

What is the status of this PR?

It is waiting for answers for questions:

What to do with the missing constants? Based on your pull request to libc, supposed answer is "wait for libc support", then "wait when nix starts depending on new libc".
Shall we get ahead of musl upstream and provide statx even on musl by using hacks?
Is overall design of the feature is OK and in line with nix? If code review comments e.g. about statx_without_path are resolved, and points above are resolved, will it be OK for merge or there will be just another round of problems?

SteveLauC · 2022-10-05T06:41:43Z

Shall we get ahead of musl upstream and provide statx even on musl by using hacks?

IMHO, this is not a good idea as musl hasn't provided a wrapper in their library. As I said in this comment, I would like to temporarily just support GNU/Linux.

Add statx

9302265

Resolves nix-rust#1649.

vi mentioned this pull request Feb 9, 2022

Feature request: wrap Linux's statx #1649

Open

asomers requested changes Mar 6, 2022

View reviewed changes

Update src/fcntl/statx.rs

2ad877c

Co-authored-by: Alan Somers <asomers@gmail.com>

asomers requested changes Mar 6, 2022

View reviewed changes

vi added 3 commits March 7, 2022 03:37

Fix some code review issues

00d7309

Remove statx_without_path

646e10d

Fix more code review issues

91eda3c

asomers reviewed Mar 7, 2022

View reviewed changes

vi added 2 commits March 7, 2022 04:35

statx: tuple struct

3f9a3df

statx: fix typo in doccomment

913aea0

SteveLauC reviewed Oct 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add statx #1660

Add statx #1660

vi commented Feb 9, 2022

asomers Mar 6, 2022

asomers Mar 6, 2022

asomers Mar 6, 2022

vi Mar 7, 2022 •

edited

asomers Mar 6, 2022

vi Mar 6, 2022

asomers Mar 6, 2022

vi Mar 7, 2022

asomers Mar 6, 2022

asomers Mar 7, 2022

asomers Mar 7, 2022

vi Mar 7, 2022

asomers Mar 7, 2022

vi Mar 7, 2022

asomers Mar 7, 2022

vi Mar 7, 2022

SteveLauC Oct 5, 2022

SteveLauC commented Oct 4, 2022 •

edited

SteveLauC Oct 5, 2022 •

edited

SteveLauC Oct 5, 2022

vi commented Oct 5, 2022

SteveLauC commented Oct 5, 2022

	/// is used as starting directory for lookups. If `dirfs` is None, current
	/// is used as starting directory for lookups. If `dirfd` is None, current

+                      }
+                  }
+                  /*

Add statx #1660

Are you sure you want to change the base?

Add statx #1660

Conversation

vi commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vi Mar 7, 2022 • edited

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

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 Oct 4, 2022 • edited

SteveLauC Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vi commented Oct 5, 2022

SteveLauC commented Oct 5, 2022

vi Mar 7, 2022 •

edited

SteveLauC commented Oct 4, 2022 •

edited

SteveLauC Oct 5, 2022 •

edited