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

Switching Sysno to usize #50

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

sectordistrict
Copy link
Contributor

Hello, I've come across this project while working through a personal project and saw the request for PR on #46, I figured to do it since I'd already read up on the codebase.
The guidelines were helpful and the codebase currently uses usize instead of Sysno, this is of course in the raw parts.
I've tested the current changes on an x86 machine, and all public apis are functioning correctly.

src/lib.rs Outdated
pub use super::syscall::syscall4;
pub use super::syscall::syscall5;
pub use super::syscall::syscall6;
pub use super::syscall::raw_syscall0;
Copy link
Owner

Choose a reason for hiding this comment

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

Small, but important, build error: raw_syscallX -> syscallX

Changing syscalls::raw::syscallX to take a usize instead of Sysno is also technically a breaking change, so I think I'll release a new major version of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems I previously only changed the function names on the x86-64 arch, and this is why its currently the only arch that's correctly building.

I also saw your other comment, so now I've reverted x86-64 instead of changing the other archs (revert the function names on x86-64 to syscall[0-6] instead of raw_syscall[0-6])

@jasonwhite
Copy link
Owner

Everything looks great except for the build error. Thanks!

/// Issues a raw system call with 0 arguments.
///
/// # Safety
///
/// Running a system call is inherently unsafe. It is the caller's
/// responsibility to ensure safety.
#[inline]
pub unsafe fn syscall0(n: Sysno) -> usize {
pub unsafe fn raw_syscall0(n: usize) -> usize {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these names need to change to raw_* actually. All architecture-specific syscalls are private (but reexported via syscalls::raw::*).

Keeping these the same should result in the least amount of breaking change. Likewise, the syscalls::raw::* names should remain the same and where the only difference is that Sysno is replaced by usize.

@sectordistrict
Copy link
Contributor Author

Just amended the previous commits, cargo tests are passing, and a small test crate I have is compiling successfully, hopefully the CI builds this time
Thanks for the feedback

@jasonwhite jasonwhite merged commit 723c446 into jasonwhite:main Jul 25, 2024
26 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