[asder8215]

View all comments

This PR implements the Metadata::from_statx for Linux MetadataExt ACP. You can create a std::fs::Metadata from a *const c_void populated by the statx syscall.

The tracking issue for this ACP is here.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, jhpratt, nia-e

[joshtriplett]

I had a couple of questions regarding to what Josh said in the ACP:

We'd also be interested in adding a constant for "the statx flags std normally requests", so that code wanting to request everything std knows about can use those.
^I wanted a little bit more clarity on what Josh meant by this.

* How should I go about adding in flags/masks for populating the `Metadata` struct with the `*const c_void` pointer containing the statx info? I'm assuming it's possible that not all of the fields from our `statxbuf` will be submitted or given to `Metadata` stat fields (since I understood that this function is supposed to be flexible), so we risk accessing something not initialized?

You can provide a single public const STATX_MASK, of type c_int, for the appropriate mask. It's the caller's responsibility to make sure std doesn't encounter uninitialized data.

* Should this function return a `Result<Metadata>`? Should I be checking whether the given `*const c_void` is a null pointer or not, and returning an `Error` if it is?

It's an unsafe function; you have to pass it a valid pointer. We could panic if we get null, as a precaution, but I'm not sure we should; I think we should just assume the pointer is valid. The function should return a Metadata.

[asder8215]

r? libs

[Darksonn]

I was planning to wait for #154981 before implementing this to avoid this clause.

View changes since the review

[asder8215]

Got you. I think the order doesn't matter right now since this function would automatically have musl support once #154981 lands as the PR updates the cfg_has_statx macro. The only thing that needs to be updated is the documentation for this function, which I'm down to keep my eyes on #154981 to see it get merged in.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Mark-Simulacrum]

Is there a reason we can't copy from the passed pointer into our stat for the size that we're expecting to be valid? Presumably that is a fixed 'header' of the passed structure of known size?

I think with either implementation strategy we need some better tests -- I think right now we're just checking that the size is copied? It seems easy to forget to update this in the future.

View changes since the review

[asder8215]

I took this from try_statx in sys/fs/unix.rs.

rust/library/std/src/sys/fs/unix.rs

Lines 218 to 234 in ff64732

	stat.st_dev = libc::makedev(buf.stx_dev_major, buf.stx_dev_minor) as _;
	stat.st_ino = buf.stx_ino as libc::ino64_t;
	stat.st_nlink = buf.stx_nlink as libc::nlink_t;
	stat.st_mode = buf.stx_mode as libc::mode_t;
	stat.st_uid = buf.stx_uid as libc::uid_t;
	stat.st_gid = buf.stx_gid as libc::gid_t;
	stat.st_rdev = libc::makedev(buf.stx_rdev_major, buf.stx_rdev_minor) as _;
	stat.st_size = buf.stx_size as off64_t;
	stat.st_blksize = buf.stx_blksize as libc::blksize_t;
	stat.st_blocks = buf.stx_blocks as libc::blkcnt64_t;
	stat.st_atime = buf.stx_atime.tv_sec as libc::time_t;
	// `i64` on gnu-x86_64-x32, `c_ulong` otherwise.
	stat.st_atime_nsec = buf.stx_atime.tv_nsec as _;
	stat.st_mtime = buf.stx_mtime.tv_sec as libc::time_t;
	stat.st_mtime_nsec = buf.stx_mtime.tv_nsec as _;
	stat.st_ctime = buf.stx_ctime.tv_sec as libc::time_t;
	stat.st_ctime_nsec = buf.stx_ctime.tv_nsec as _;

However, just to clarify the stat variable you're referring to is a libc::stat64 while our buf is a libc::statx looking at statx and stat64 struct representation:

pub struct statx {
    pub stx_mask: crate::__u32,
    pub stx_blksize: crate::__u32,
    pub stx_attributes: crate::__u64,
    pub stx_nlink: crate::__u32,
    pub stx_uid: crate::__u32,
    pub stx_gid: crate::__u32,
    pub stx_mode: crate::__u16,
    __statx_pad1: Padding<[crate::__u16; 1]>,
    pub stx_ino: crate::__u64,
    pub stx_size: crate::__u64,
    pub stx_blocks: crate::__u64,
    pub stx_attributes_mask: crate::__u64,
    pub stx_atime: statx_timestamp,
    pub stx_btime: statx_timestamp,
    pub stx_ctime: statx_timestamp,
    pub stx_mtime: statx_timestamp,
    pub stx_rdev_major: crate::__u32,
    pub stx_rdev_minor: crate::__u32,
    pub stx_dev_major: crate::__u32,
    pub stx_dev_minor: crate::__u32,
    pub stx_mnt_id: crate::__u64,
    pub stx_dio_mem_align: crate::__u32,
    pub stx_dio_offset_align: crate::__u32,
    __statx_pad3: Padding<[crate::__u64; 12]>,
}

pub struct stat64 {
    pub st_dev: crate::dev_t,
    pub st_ino: crate::ino64_t,
    pub st_nlink: crate::nlink_t,
    pub st_mode: crate::mode_t,
    pub st_uid: crate::uid_t,
    pub st_gid: crate::gid_t,
    __pad0: Padding<c_int>,
    pub st_rdev: crate::dev_t,
    pub st_size: off_t,
    pub st_blksize: crate::blksize_t,
    pub st_blocks: crate::blkcnt64_t,
    pub st_atime: crate::time_t,
    pub st_atime_nsec: i64,
    pub st_mtime: crate::time_t,
    pub st_mtime_nsec: i64,
    pub st_ctime: crate::time_t,
    pub st_ctime_nsec: i64,
    __reserved: Padding<[i64; 3]>,
}

I don't think we can copy the passed in statx pointer (which we assume the user gave us a pointer with the same representations as libc::statx) directly and create a stat64 object out of this. For example, stat64.st_dev relies on statx.stx_dev_major and statx.stx_dev_minor, which we anyways we need to use libc::makedev to properly create the dev_t value.

Also, Metadata is wrapper around FileAttr, which on unix it's represented as:

pub struct FileAttr {
        stat: stat64,
        statx_extra_fields: Option<StatxExtraFields>,
}

So all the stat64 and statx extra information are copied separately as a result to populate FileAttr.

[Mark-Simulacrum]

Ah, I see, they're different structs, I missed that when skimming the impl. Maybe we can re-use try_statx?

My main worry here is that it seems pretty easy for the two implementations to diverge, which is probably not great -- at best, it's missing features for one of them, at worst unsoundness if we assume something is always initialized (but isn't actually).

I suppose most (all?) of the fields are just integers though so maybe the zero-init makes it mostly harmless...

[asder8215]

I think a shared function can be created for both try_statx and this Metadata::from_statx since they both are just creating a FileAttr at the end of the day. I can modify my implementation of FileAttr::from_statx to take in a libc::statx value, do the same thing in populating a stat64 and StatxExtraFields, and return a FileAttr struct out of this.

If that sounds good to you, should I mark the FileAttr::from_statx function unsafe?

[Mark-Simulacrum]

If it has a safety precondition, then yes. I think it would.

[joshtriplett]

We talked about this again at length in today's @rust-lang/libs-api meeting.

Our tentative conclusion, partly informed by conversations with the Rust-for-Linux folks, was that any kind of extension of the length of struct statx would be non-trivial and error-prone, and would likely require either an added length field or a new syscall, to avoid breaking existing userspace. (The manpage does say "don't pass flags you don't understand", but despite that, it seems error-prone for the kernel to assume userspace passed a sufficient buffer.)

It'd be helpful to get that re-confirmed by the Rust-for-Linux folks.

[Mark-Simulacrum]

I added it as a question to the tracking issue. I think this is in reasonable shape to be merged as-is, and we can iterate more before this gets stabilized if needed.

@bors squash

[rust-bors]

🔨 3 commits were squashed into 5a417e2.

[Mark-Simulacrum]

@bors r+

View changes since this review

[rust-bors]

📌 Commit 5a417e2 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

[jhpratt]

@bors r-

#158533 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#158533), which was unapproved.

View changes since this unapproval

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[asder8215]

Since the bump to libc 0.2.191 introduces unconditional musl support on statx, STATX_BASIC_STATS, and STATX_BTIME, this should hopefully compile and pass.

We do have to wait for #159540 to make the changes to cfg_has_statx to match on linux platforms (instead of linux platforms with a target env of gnu). I could make the change here, but I think it's better for that PR to merge in first instead.

In the meantime, I'll mark this as ready once/if CI turns green.

[asder8215]

@rustbot ready

Note again: this PR is ready, but we should wait for #159540 to land first.