Skip to content

Conversation

@mxinden
Copy link
Contributor

@mxinden mxinden commented Sep 30, 2020

Previously a27a68d wrongly assumed libc::rlim_t to be a u64 on all
platforms. Instead the following is the case:

  • x86_64-unknown-linux-gnu: u64
  • x86_64-apple-darwin: u64
  • i686-unknown-linux-musl: u64
  • i686-unknown-linux-gnu: u32
  • ...

There are different options in order to compile on all platforms:

  • Return usize. While usize would work for most platforms,
    i686-unknown-linux-musl is an exception. Thus casting would be needed
    which introduces yet another way to panic. In addition usize should be
    used to index into collections, not so much to represent actual data.

  • Return libc::rlim_t. This type is not present on all platforms. In
    addition exposing a libc type in the public interface unnecessarily
    ties us to libc.

  • Return u32. Would require a fallible cast on all u64 platforms.

  • Return u64. Supported on all platforms. It is not the most space
    efficient option.

Given that memory efficiency (u32 vs u64) is irrelevant in this
context, compared to the involved system calls, this commit converts the
limit to u64 before returning for all platforms.

Fixes #5.

Previously a27a68d wrongly assumed `libc::rlim_t` to be a `u64` on all
platforms. Instead the following is the case:

- x86_64-unknown-linux-gnu: u64
- x86_64-apple-darwin: u64
- i686-unknown-linux-musl: u64
- i686-unknown-linux-gnu: u32
- ...

There are different options in order to compile on all platforms:

- Return `usize`. While `usize` would work for most platforms,
`i686-unknown-linux-musl` is an exception. Thus casting would be needed
which introduces yet another way to panic. In addition `usize` should be
used to index into collections, not so much to represent actual data.

- Return `libc::rlim_t`. This type is not present on all platforms. In
addition exposing a `libc` type in the public interface unnecessarily
ties us to `libc`.

- Return `u32`. Would require a fallible cast on all `u64` platforms.

- Return `u64`. Supported on all platforms. It is not the most space
efficient option.

Given that memory efficiency (`u32` vs `u64`) is irrelevant in this
context, compared to the involved system calls, this commit converts the
limit to `u64` before returning for all platforms.
@mxinden
Copy link
Contributor Author

mxinden commented Sep 30, 2020

Tested on the following platforms. Let me know in case I am missing one.

➜  fdlimit git:(convert-to-u64) cargo build --target x86_64-unknown-linux-gnu

   Compiling libc v0.2.76
   Compiling fdlimit v0.2.0 (/home/mxinden/code/github.com/paritytech/fdlimit)
    Finished dev [unoptimized + debuginfo] target(s) in 0.73s
➜  fdlimit git:(convert-to-u64) cargo build --target x86_64-apple-darwin

   Compiling fdlimit v0.2.0 (/home/mxinden/code/github.com/paritytech/fdlimit)
    Finished dev [unoptimized + debuginfo] target(s) in 0.43s
➜  fdlimit git:(convert-to-u64) cargo build --target i686-unknown-linux-musl

   Compiling fdlimit v0.2.0 (/home/mxinden/code/github.com/paritytech/fdlimit)
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
➜  fdlimit git:(convert-to-u64) cargo build --target i686-unknown-linux-gnu

   Compiling fdlimit v0.2.0 (/home/mxinden/code/github.com/paritytech/fdlimit)
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
➜  fdlimit git:(convert-to-u64) cargo build --target armv7-unknown-linux-gnueabihf

    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

@decathorpe
Copy link

Oh, it's really weird that it's not even consistent across 32-bit targets ...
So, always converting to the larger data type (u64) sounds good to me. 👍

@mxinden
Copy link
Contributor Author

mxinden commented Oct 1, 2020

@NikVolf could you give this a review?

@mxinden mxinden requested a review from NikVolf October 1, 2020 06:51
@NikVolf NikVolf merged commit 1cec4ad into paritytech:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crate fails to comile on 32-bit architectures

3 participants