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

fcntl adding apple F_PREALLOCATE flag. #2393

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented May 5, 2024

What does this PR do

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

@devnexen devnexen marked this pull request as ready for review May 5, 2024 10:27
@SteveLauC
Copy link
Member

SteveLauC commented May 5, 2024

I am working on the CI issues, they are not related to this PR

Update: CI fixed in #2394, you can rebase your branch now:)

@SteveLauC
Copy link
Member

Gentle ping on this PR, would you like to rebase and finish it?

@devnexen
Copy link
Contributor Author

seems libc 0.2.154 not yet available (just the tag on the repo but not yet published maybe) ?

@SteveLauC
Copy link
Member

seems libc 0.2.154 not yet available (just the tag on the repo but not yet published maybe) ?

Right, it has been yanked, we have to wait for rust-lang/libc#3682

@SteveLauC
Copy link
Member

The issue has been fixed, please rebase your branch:)

src/fcntl.rs Outdated
@@ -692,6 +692,9 @@ pub enum FcntlArg<'a> {
/// Return the full path without firmlinks of the fd.
#[cfg(apple_targets)]
F_GETPATH_NOFIRMLINK(&'a mut PathBuf),
/// Pre-allocate storage with different policies on fd.
#[cfg(apple_targets)]
F_PREALLOCATE(&'a libc::fstore_t),
Copy link
Member

Choose a reason for hiding this comment

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

The associated buffer has an OUT field:

typedef struct fstore {
	    u_int32_t fst_flags;      /* IN: flags word */
	    int       fst_posmode;    /* IN: indicates offset field */
	    off_t     fst_offset;     /* IN: start of the region */
	    off_t     fst_length;     /* IN: size of the region */
	    off_t     fst_bytesalloc; /* OUT: number of bytes allocated */
} fstore_t;

i.e., the buffer can be written, if we use an immutable reference here, then UB could happen

Ref: https://keith.github.io/xcode-man-pages/fcntl.2.html#F_PREALLOCATE

Copy link
Member

Choose a reason for hiding this comment

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

I just tried implementing this on my mac, I think we should create a helper type for those IN fields, and return the OUT field through fcntl().

For that fst_falgs field, we would create a bitflags struct, for fst_posmode, a Rust enum would suffice.

And F_ALLOCATEPERSIST is not available in libc, we should add it first.

Copy link
Member

@SteveLauC SteveLauC May 19, 2024

Choose a reason for hiding this comment

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

libc PR filed: rust-lang/libc#3713

Update: PR merged

@@ -800,6 +800,9 @@ pub enum FcntlArg<'a> {
/// Issue an advisory read async with no copy to user
#[cfg(apple_targets)]
F_RDADVISE(libc::radvisory),
/// Pre-allocate storage with different policies on fd.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we shoud document the reason why a mutable reference is needed here? i.e., there is a OUT field in the associated sturcutre🤔

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SteveLauC
Copy link
Member

Looks like you need to resolve the conflict in src/fcntl.rs.

@SteveLauC SteveLauC added this pull request to the merge queue Sep 6, 2024
Merged via the queue into nix-rust:master with commit 66be012 Sep 6, 2024
36 checks passed
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