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

journald: fix memfd_create_syscall for 32 bit targets #1982

Merged
merged 2 commits into from Mar 14, 2022

Conversation

chrta
Copy link
Contributor

@chrta chrta commented Mar 10, 2022

Motivation

I would like to use tracing-journald on a 32 bit target (armv7).

Solution

On 32 bit targets (e.g. armv7) the syscall in memfd_create_syscall()
returns an i32, so this compilation error is printed:

| error[E0308]: mismatched types
|   --> .../tracing-journald-0.2.3/src/memfd.rs:27:9
|    |
| 25 |   fn memfd_create_syscall(flags: c_uint) -> i64 {
|    |                                             --- expected `i64`
|    |                                           because of return type
| 26 |       unsafe {
| 27 | /         syscall(
| 28 | |             SYS_memfd_create,
| 29 | |             "tracing-journald\0".as_ptr() as *const c_char,
| 30 | |             flags,
| 31 | |         )
|    | |_________^ expected `i64`, found `i32`
|    |
| help: you can convert an `i32` to an `i64`
|    |
| 31 |         ).into()
|    |          +++++++
|
| For more information about this error, try `rustc --explain E0308`.
| error: could not compile `tracing-journald` due to previous error
|

@chrta chrta requested review from hawkw, davidbarsky and a team as code owners March 10, 2022 12:47
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

While this works, I don't think it's really the most correct solution. The memfd_create system call returns a C int type, so I think that, instead of calling .into() on the return value, we should just change the definition of the memfd_create_syscall function so that, rather than returning an i64, it returns the c_int type from libc. This will always be the correct type on the current platform, and we can remove the into() call.

@chrta
Copy link
Contributor Author

chrta commented Mar 10, 2022

Unfortunately c_int type from libc does not work, since it is always i32, also for 64 bit targets. So in this case compilation for 64 bit targets fails.

Edit: But i would also like a better solution. But i did not find any type that is either a i32 or i64 depending on the target platform.

@hawkw
Copy link
Member

hawkw commented Mar 10, 2022

Unfortunately c_int type from libc does not work, since it is always i32, also for 64 bit targets. So in this case compilation for 64 bit targets fails.

Edit: But i would also like a better solution. But i did not find any type that is either a i32 or i64 depending on the target platform.

Ah, sorry, my bad, I meant to say that it should be c_long, rather than c_int. The syscall function returns c_long, and I believe the corresponding linux libc function is also defined to return a long.

@ZanderBrown
Copy link

Yes, syscall(2) returns long however it's also true that memfd_create(2) is int

Thus it should be perfectly safe to force the c_long into c_int, in fact RawFd requires you to. But equally this should definitely be implemented in terms of c_long and c_int rather than plain rust types.

On 32 bit targets (e.g. armv7) the syscall in memfd_create_syscall()
returns an i32, so this compilation error is printed:

| error[E0308]: mismatched types
|   --> .../tracing-journald-0.2.3/src/memfd.rs:27:9
|    |
| 25 |   fn memfd_create_syscall(flags: c_uint) -> i64 {
|    |                                             --- expected `i64`
|    |                                           because of return type
| 26 |       unsafe {
| 27 | /         syscall(
| 28 | |             SYS_memfd_create,
| 29 | |             "tracing-journald\0".as_ptr() as *const c_char,
| 30 | |             flags,
| 31 | |         )
|    | |_________^ expected `i64`, found `i32`
|    |
| help: you can convert an `i32` to an `i64`
|    |
| 31 |         ).into()
|    |          +++++++
|
| For more information about this error, try `rustc --explain E0308`.
| error: could not compile `tracing-journald` due to previous error
|

This commit fixes this issue.
@chrta chrta force-pushed the fix-memfd-create-syscall-ret branch from 7bf53bf to 30ea42a Compare March 10, 2022 20:23
@chrta
Copy link
Contributor Author

chrta commented Mar 10, 2022

Thank you for the explanations, i am completely new to rust, so this is a little bit more difficult for me. I retried with converting the returned c_long into a c_int, because in the create() function the fd is expected to be convertible into RawFd.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

the cast to c_int seems fine to me, i think

@hawkw hawkw requested a review from Ralith March 11, 2022 17:05
Copy link
Collaborator

@Ralith Ralith 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!

@hawkw hawkw enabled auto-merge (squash) March 12, 2022 22:12
@hawkw hawkw merged commit b16dc03 into tokio-rs:master Mar 14, 2022
@chrta chrta deleted the fix-memfd-create-syscall-ret branch March 14, 2022 19:06
hawkw pushed a commit that referenced this pull request Mar 17, 2022
On 32 bit targets (e.g. armv7) the syscall in memfd_create_syscall()
returns an i32, so this compilation error is printed:

| error[E0308]: mismatched types
|   --> .../tracing-journald-0.2.3/src/memfd.rs:27:9
|    |
| 25 |   fn memfd_create_syscall(flags: c_uint) -> i64 {
|    |                                             --- expected `i64`
|    |                                           because of return type
| 26 |       unsafe {
| 27 | /         syscall(
| 28 | |             SYS_memfd_create,
| 29 | |             "tracing-journald\0".as_ptr() as *const c_char,
| 30 | |             flags,
| 31 | |         )
|    | |_________^ expected `i64`, found `i32`
|    |
| help: you can convert an `i32` to an `i64`
|    |
| 31 |         ).into()
|    |          +++++++
|
| For more information about this error, try `rustc --explain E0308`.
| error: could not compile `tracing-journald` due to previous error
|

This commit fixes this issue.
@hawkw hawkw mentioned this pull request Mar 17, 2022
hawkw added a commit that referenced this pull request Mar 17, 2022
On 32 bit targets (e.g. armv7) the syscall in memfd_create_syscall()
returns an i32, so this compilation error is printed:

| error[E0308]: mismatched types
|   --> .../tracing-journald-0.2.3/src/memfd.rs:27:9
|    |
| 25 |   fn memfd_create_syscall(flags: c_uint) -> i64 {
|    |                                             --- expected `i64`
|    |                                           because of return type
| 26 |       unsafe {
| 27 | /         syscall(
| 28 | |             SYS_memfd_create,
| 29 | |             "tracing-journald\0".as_ptr() as *const c_char,
| 30 | |             flags,
| 31 | |         )
|    | |_________^ expected `i64`, found `i32`
|    |
| help: you can convert an `i32` to an `i64`
|    |
| 31 |         ).into()
|    |          +++++++
|
| For more information about this error, try `rustc --explain E0308`.
| error: could not compile `tracing-journald` due to previous error
|

This commit fixes this issue.

Co-authored-by: Christian Taedcke <hacking@taedcke.com>
hawkw added a commit that referenced this pull request Mar 17, 2022
# 0.2.4 (March 17, 2022)

### Fixed

- Fixed compilation error in `memfd_create_syscall` on 32-bit targets
  ([#1982])

Thanks to new contributor @chrta for contributing to this release!

[#1982]: #1982
davidbarsky pushed a commit that referenced this pull request Mar 17, 2022
# 0.2.4 (March 17, 2022)

### Fixed

- Fixed compilation error in `memfd_create_syscall` on 32-bit targets
  ([#1982])

Thanks to new contributor @chrta for contributing to this release!

[#1982]: #1982
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.

None yet

4 participants