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

Declare statfs MAGIC constants as c_uint on s390x #1999

Merged
merged 1 commit into from
Dec 21, 2020
Merged

Declare statfs MAGIC constants as c_uint on s390x #1999

merged 1 commit into from
Dec 21, 2020

Conversation

Jakob-Naucke
Copy link
Contributor

Hi, I work in container development on Linux on Z at IBM.
On s390x (IBM Z) in GNU/Linux, a statfs f_type is 4 bytes wide, contrary to 8 bytes on most architectures.
This is already implemented in libc:

$ grep -r f_type src/unix/linux_like/linux/gnu/b64 | uniq
src/unix/linux_like/linux/gnu/b64/sparc64/mod.rs:        pub f_type: ::__fsword_t,
src/unix/linux_like/linux/gnu/b64/aarch64/mod.rs:        pub f_type: ::__fsword_t,
src/unix/linux_like/linux/gnu/b64/powerpc64/mod.rs:        pub f_type: ::__fsword_t,
src/unix/linux_like/linux/gnu/b64/riscv64/mod.rs:        pub f_type: ::c_long,
src/unix/linux_like/linux/gnu/b64/mips64/mod.rs:        pub f_type: ::c_long,
src/unix/linux_like/linux/gnu/b64/s390x.rs:        pub f_type: ::c_uint,             # s390x is uint
src/unix/linux_like/linux/gnu/b64/x86_64/mod.rs:        pub f_type: ::__fsword_t,

However, the *_MAGIC constants (such as EXT4_SUPER_MAGIC) in src/unix/linux_like/linux/gnu/mod.rs are defined as c_long, when they should be c_uint on s390x.
This ends up biting me here.
Thus, I suggest the attached change to only define these constants for architectures other than s390x and instead define them as uint for s390x.
This is safe since none of the constants are any wider than 32bit.
Please let me know if you think this could be done more elegantly.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@JohnTitor
Copy link
Member

Makes sense!

Please let me know if you think this could be done more elegantly.

You could use cfg_if macro for that case.

Previously, statfs MAGIC constants like EXT4_SUPER_MAGIC were defined as c_long
for linux-gnu, whereas a statfs f_type is a c_uint on s390x. This commit
exempts these definitions from s390x and instead defines these constants as
c_uint on s390x. This is safe as none of these constants are wider than 32bit.

Signed-off-by: Jakob Naucke <jakob.naucke@ibm.com>
@Jakob-Naucke
Copy link
Contributor Author

@JohnTitor more like this?

@JohnTitor
Copy link
Member

more like this?

Yeah, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2020

📌 Commit 3d09b9f has been approved by JohnTitor

@bors
Copy link
Contributor

bors commented Dec 21, 2020

⌛ Testing commit 3d09b9f with merge c7b9771...

@bors
Copy link
Contributor

bors commented Dec 21, 2020

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: JohnTitor
Pushing c7b9771 to master...

@bors bors merged commit c7b9771 into rust-lang:master Dec 21, 2020
@Jakob-Naucke Jakob-Naucke deleted the fix-statfs-magic-s390x branch December 21, 2020 17:21
@Jakob-Naucke
Copy link
Contributor Author

Jakob-Naucke commented Dec 21, 2020

Awesome, thank you! It would be good for this fix to be released to fix Kata Containers on s390x. @JohnTitor can I create a bump or do you feel the change is too insignificant?

@JohnTitor
Copy link
Member

@Jakob-Naucke You can create a PR to make a release following the contributing guide: https://github.com/rust-lang/libc/blob/master/CONTRIBUTING.md#releasing-your-change-to-cratesio
But there are some PRs that are waiting for review and I may be unavailable during this holiday season.

Jakob-Naucke added a commit to Jakob-Naucke/kata-containers that referenced this pull request Mar 29, 2021
to pull in the chain of rust-lang/libc#1999,
nix-rust/nix#1372, and
kata-containers/cgroups-rs#38. This adds statfs
constants on s390x. cgroups-rs 0.2.4 also contains this fix, but let's
move to the latest 0.2.5 right away.

Fixes: kata-containers#1204

Signed-off-by: Jakob Naucke <jakob.naucke@ibm.com>
Jakob-Naucke added a commit to Jakob-Naucke/kata-containers that referenced this pull request Mar 29, 2021
statfs f_types are long on most architectures, but not on s390x, where
they are uint. Following the fix in rust-lang/libc at
rust-lang/libc#1999, the custom defined
PROC_SUPER_MAGIC must be updated in a similar way.

Fixes: kata-containers#1204

Signed-off-by: Jakob Naucke <jakob.naucke@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants