Remove use of locale-specific strerror_r.

[briansmith]

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.

[briansmith]

Even if the bytes do not contain any invalid UTF-8 sequences, they might not be encoding UTF-8.

[josephlr]

Is there an actual example of this being an issue on a unix target?

[briansmith]

No offense, but I don't participate in that kind of thinking. Instead it would be better to have a proof that it isn't an issue.

[josephlr]

Oh no offense taken, I think this is a good point (especially when it comes to stuff like the safety of code or cryptographic security where I 100% agree). Sorry for being a little short. I just meant to note that an implementation returning UTF8 bytes that aren't UTF8 only causes the display implementation to show a suboptimal error string.

However, after looking more into strerror_r, I agree that we shouldn't be assuming that it is well behaved on arbitrary unix targets. This is libstd's job, not ours. See my suggestion below.

[josephlr]

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]

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]

I agree with the @josephlr's suggestion above.

[briansmith]

 #[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]

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.

[josephlr]

Thanks for the fix!!