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

adjusting statvfs struct to have 32-bit values for 32-bit machines #784

Closed
wants to merge 1 commit into from

Conversation

noyez
Copy link

@noyez noyez commented Oct 19, 2017

This PR fixes the statvfs struct on 32-bit machines (my test bed was an R-PI). By default the statvfs struct uses mostly u64 values on all platforms. This PR uses u32 on 32-bit platforms and u64 on 64-bit platforms. Below is a short demonstration of the bug.

Given the following Rust program:

extern crate nix;
fn main() {
    let stat = nix::sys::statvfs::vfs::Statvfs::for_path("/").unwrap();
    println!("Total blocks: {}", stat.f_blocks);
    println!("Total blocks (as 32): {}", stat.f_blocks as u32);
}

On my R-PI output:

Total blocks: 836247313342446
Total blocks (as 32): 942062

Consider the following python command:

python -c "import os;  statvfs = os.statvfs('/'); print(statvfs.f_blocks)"

On my R-PI:

942062

@Susurrus
Copy link
Contributor

This actually shouldn't be something nix does, declaring libc datatypes. If you search the libc repo you'll see that statvfs and statvfs64 is already declared for us, we should just make nix use a Newtype around those structs instead of declaring them directly. See the work linked to from #748.

Additionally, this may be related to #743, which is about getting the 64-bit version implemented.

@Susurrus
Copy link
Contributor

Actually, this is already fixed in git, doing exactly what I said, so I don't think this PR is relevant anymore. Can you check the latest code and confirm that this is resolved for you?

@Susurrus
Copy link
Contributor

See 6ae3f31 for the commit that already fixed this.

@noyez
Copy link
Author

noyez commented Oct 24, 2017

Yup, i just tested master on my platform, works as expected.

I swear i tried master before i created the PR, but it didn't compile at that instant (for some unmemorable reason). Eitherway, it works now.

@noyez noyez closed this Oct 24, 2017
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