rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
264 stars 166 forks source link

Remove use of locale-specific `strerror_r`. #440

Closed briansmith closed 1 month ago

briansmith commented 1 month ago

strerror_r doesn't necessarily return UTF-8 but we assume it does.

strerror_r is locale-sensitive but we are better off not being locale-sensitive.

strerror_r requires us to use libc but increasingly we want to avoid libc, e.g. to support x86_64-unknown-linux-none.

Just remove the use of the function.

josephlr commented 1 month ago

I don't think we should remove this. It is quite nice for the Display implementation of an Error to actually print out what the Error means. It doesn't hurt to have it, and if strerror_r happens to return non-UTF8 bytes (which I've never seen happen, but I agree it might be theoretically possible), we just don't print the error string.

Is there a reason not to just use the cfg to exclude targets which don't use libc?

Edit: I think we can actually remove use of stderror_r while still keeping nice descriptions.

josephlr commented 1 month ago

After looking at this some more (and reading more about stderror_r), I actually think we should remove use of libc::strerror_r here. If std is enabled, we can just use std::io::Error to get a good Debug/Display implementation by doing something like:

impl fmt::Display for Error {
  fn fmt(&self, f: &mut Formatter) {
    if let Some(errno) = self.raw_os_error() {
      #[cfg(feature = "std")]
      std::io::Error::from_raw_os_error(error).fmt(f)
      #[cfg(not(feature = "std"))]
      write!(f, "OS error {}", errno)
    } else {
      ...
    }
  }
}
newpavlov commented 1 month ago

I agree with the @josephlr's suggestion above.

briansmith commented 1 month ago
 #[cfg(feature = "std")]
      std::io::Error::from_raw_os_error(error).fmt(f)

This would mean that the output would depend on the std feature flag. However, this does mean that anything that embeds getrandom::Error in its own structures (errors) will then have its output depend on the std feature flag for getrandom? That seems a bit unfortunate. I personally think less conditional logic is better than avoiding the need to look up the meaning of an error code manually in the extremely rare circumstances anybody needs to do so.

newpavlov commented 1 month ago

Arguably, the main reason for the Error type existence is improved formatting of user-facing error messages. So I think that improving it with a tiny bit of conditional logic is fine.