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

Add a rustix::fs::STATX_ATTR_MOUNT_ROOT constant. #1198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JustSoup312
Copy link

Add a STATX_ATTR_MOUNT_ROOT constant for Linux with musl libc compatibility.

(Note: This is my first contribution to rustix, so I assume that I may have placed const declarations in the wrong place. I will gladly fix any issues with this MR. Thank you!)

@JustSoup312 JustSoup312 marked this pull request as ready for review October 28, 2024 02:43
@JustSoup312 JustSoup312 marked this pull request as draft October 28, 2024 02:47
@JustSoup312 JustSoup312 force-pushed the musl-statx-compat branch 10 times, most recently from ca42248 to aef0ca7 Compare October 28, 2024 03:50
Add a `STATX_ATTR_MOUNT_ROOT` constant for musl libc compat
@JustSoup312 JustSoup312 marked this pull request as ready for review October 28, 2024 04:07
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall it looks good; a few comments below:

#[cfg(all(linux_kernel, target_env = "musl"))]
pub(crate) const STATX_ATTR_MOUNT_ROOT: c_int = 0x2000;
#[cfg(all(linux_kernel, target_env = "gnu"))]
pub(crate) const STATX_ATTR_MOUNT_ROOT: c_int = libc::STATX_ATTR_MOUNT_ROOT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module does use libc::* at the top, so this line is unnecessary.

AT_FDCWD, NFS_SUPER_MAGIC, O_LARGEFILE, PROC_SUPER_MAGIC, STATX_ATTR_MOUNT_ROOT, UTIME_NOW,
UTIME_OMIT, XATTR_CREATE, XATTR_REPLACE,
};
#[cfg(not(any(target_env = "musl", target_env = "gnu")))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The src/backend/linux_raw directory doesn't use libc bindings for most things, so we should be able to just add STATX_ATTR_MOUNT_ROOT to the use below instead of adding a new use here.

@@ -10,6 +10,11 @@ use backend::fs::syscalls::statx as _statx;
#[cfg(not(feature = "linux_4_11"))]
use compat::statx as _statx;

/// `STATX_ATTR_MOUNT_ROOT`—Address of the root of a mount
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a pub const and re-exported in mod.rs, it shouldn't need allow(dead_code).

@@ -116,6 +116,8 @@ pub use sendfile::sendfile;
pub use special::*;
#[cfg(linux_kernel)]
pub use statx::statx;
#[cfg(all(linux_kernel, any(target_env = "musl", target_env = "gnu")))]
pub use statx::STATX_ATTR_MOUNT_ROOT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we can do to make this code a little tidier is just change the use statx::statx above to be use statx::*;, instead of adding a new use, so that we don't have to repeat all the cfgs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants