Skip to content

Replace libc in Linux with rustix#41

Merged
Stebalien merged 5 commits intoStebalien:masterfrom
forkgull:rustix
Dec 9, 2023
Merged

Replace libc in Linux with rustix#41
Stebalien merged 5 commits intoStebalien:masterfrom
forkgull:rustix

Conversation

@notgull
Copy link
Copy Markdown
Contributor

@notgull notgull commented Nov 25, 2023

rustix is a crate which provides I/O-safe wrappers around raw system
calls on Linux and libc on other Unixes. This PR makes it so the
functions in this crate call the rustix functions.

This has a number of benefits. It removes a substantial amount of
unsafe code from this crate. In addition, raw syscalls are often more
efficient than libc calls; sometimes they can even be inlined into
their calling functions.

Two compromises were made in this PR:

  • This crate does not have I/O safety yet, while rustix does. So I
    borrow the raw file descriptor for now to avoid a breaking change.
  • The BSD equivalents to the Xattr functions are not yet exposed in
    Rustix. So I still use libc in the BSD backend.

Closes #40

Copy link
Copy Markdown
Owner

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM.

N: AsRef<OsStr>,
{
util::extract_noattr(sys::get_fd(self.as_raw_fd(), name.as_ref()))
// SAFETY: Implement I/O safety later.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Note: this should be safe as-is as we don't keep the file descriptor around. I.e., the API is unsafe, but the usage is safe.

@Stebalien
Copy link
Copy Markdown
Owner

Could you rebase on master? That should cause CI to run.

@Stebalien
Copy link
Copy Markdown
Owner

It looks like we're going to need to do some casting on macos: https://github.com/Stebalien/xattr/actions/runs/6991478958/job/19022083927.

rustix is a crate which provides I/O-safe wrappers around raw system
calls on Linux and libc on other Unixes. This PR makes it so the
functions in this crate call the rustix functions.

This has a number of benefits. It removes a substantial amount of
unsafe code from this crate. In addition, raw syscalls are often more
efficient than libc calls; sometimes they can even be inlined into
their calling functions.

Two compromises were made in this PR:

- This crate does not have I/O safety yet, while rustix does. So I
  borrow the raw file descriptor for now to avoid a breaking change.
- The BSD equivalents to the Xattr functions are not yet exposed in
  Rustix. So I still use libc in the BSD backend.

Closes Stebalien#40

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Copy Markdown
Contributor Author

notgull commented Nov 26, 2023

It looks like we're going to need to do some casting on macos: https://github.com/Stebalien/xattr/actions/runs/6991478958/job/19022083927.

I've fixed this issue.


// Convert an `&mut [u8]` to an `&mut [c_char]`
#[inline]
fn as_list_buffer(buf: &mut [u8]) -> &mut [c_char] {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

nit: as_c_buffer?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

(I'm just not sure what "list buffer" means here).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The buffer is for listxattr, I've renamed it accordingly.

@notgull
Copy link
Copy Markdown
Contributor Author

notgull commented Nov 26, 2023

Not sure what the errors on BSD/MacOS mean. I'll investigate this later.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Copy Markdown
Contributor Author

notgull commented Dec 3, 2023

It's later now, I've investigated this. Both macOS and FreeBSD looks good to me, at least when I try them in a VM.

@notgull
Copy link
Copy Markdown
Contributor Author

notgull commented Dec 9, 2023

@Stebalien When do you think you can review this PR?

@Stebalien Stebalien merged commit fce3cc2 into Stebalien:master Dec 9, 2023
@Stebalien
Copy link
Copy Markdown
Owner

Thanks for the reminder!

@Stebalien
Copy link
Copy Markdown
Owner

And released as v1.1.0.

@notgull notgull deleted the rustix branch December 9, 2023 06:12
Stebalien added a commit that referenced this pull request Dec 9, 2023
This reverts commit fce3cc2.

Unfortunately, this turned out to be a breaking change #44.
Stebalien added a commit that referenced this pull request Dec 9, 2023
This reverts commit fce3cc2.

Unfortunately, this turned out to be a breaking change #44.
Stebalien added a commit that referenced this pull request Dec 9, 2023
Stebalien pushed a commit that referenced this pull request Dec 9, 2023
notgull added a commit to forkgull/xattr that referenced this pull request Dec 9, 2023
This reverts commit 2cf71bc.

Signed-off-by: John Nunley <dev@notgull.net>
Stebalien pushed a commit that referenced this pull request Dec 10, 2023
* Reapply "Replace libc in Linux with rustix (#41)"

This reverts commit 2cf71bc.

* Bump rustix to 0.38.28

This also adds rustix's net feature to dev dependencies, which should
hopefully prevent the miscompilation.

---------

Signed-off-by: John Nunley <dev@notgull.net>
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.

Port to use rustix

2 participants