Skip to content

Add std::os::set_errno as a counterpart to std::os::errno #17265

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

Closed
wants to merge 1 commit into from

Conversation

donkopotamus
Copy link
Contributor

This pull request adds a std::os::set_errno function to allow rust to manipulate the thread's errno value. In my particular case, I am needing to do this when interacting with a c-library I am wrapping.

Note I don't have rust building on a Windows platform currently so have been unable to test my implementation for [cfg(windows)]


#[cfg(unix)]
/// Returns the platform-specific value of errno
pub fn errno() -> int {
Copy link
Member

Choose a reason for hiding this comment

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

While you're at it, the return types of errno on unix and windows should be the same, not different. Could you change windows to returning an int as well? I think that most consider errno as being a signed integer as opposed to an unsigned one.

Additionally, set_errno for windows would be updated to take an int instead of a uint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those should be c_int and not int or uint.

Copy link
Member

Choose a reason for hiding this comment

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

No, they should not. We do not expose C types at the std layer where c_int is a platform-specific definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

errno() is mostly (only?!) useful with the values in libc::consts::os::posix88 which are all c_int. If you don't want c_int in the stdlib then maybe the safe version of errno and set_errno should be moved to libc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to expose errno() in std as an int, but have it be a wrapper around a c_int-based errno() in libc, and then set_errno() can go into libc.

The reasoning is that reporting the errno in error messages is not an uncommon thing, so having read-only access from std makes sense. Setting errno is much rarer, and presumably if you need that you're going to be doing other FFI work as well, so using libc is fine.

Copy link
Member

Choose a reason for hiding this comment

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

@thestinger Right; that's exactly what I said.

Copy link
Contributor

Choose a reason for hiding this comment

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

If Rust's int type is ever smaller than a C compiler's int when compiling for the same target, then either the C compiler is broken, or Rust is. More generally, under that circumstance, I think we'd have more issues than just errno.

Given that, and since errno is not defined to be any specific bit width but instead defined to be the C int type, I think it's much more natural to expose errno from std as int instead of trying to claim it's some particular bit width. Yes, it's not a perfect match, but it seems like a better choice than i32.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that my above comment is predicated on std only exposing errno as a read-only value, and libc exposing errno as a read/write value using c_int, as stated in my previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

A C compiler with a 32-bit int type and 16-bit pointers is not "broken". The same applies to one with a 64-bit int type and 32-bit pointers. If a compiler tied int to general purpose register size, it would be 64-bit on x32 while pointers (and thus int and uint in Rust) are 32-bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcrichton

We do not expose C types at the std layer

You're already doing this by re-exporting CString which takes c_char.

@donkopotamus
Copy link
Contributor Author

Some possibly relevant information about the current usage of errno.

  • errno spends a lot of its time either being casted, or being compared to other casted things (eg things like os::errno() != libc::ERROR_IO_PENDING as uint
    • and its inconsistent ... sometimes libc::ERROR* is cast to uint, sometimes to int
  • src/libnative/io/process.rs:586 seems to make an implicit assumption that errno is an i32
                let mut bytes = [0, ..4];
                return match input.inner_read(bytes) {
                    Ok(4) => {
                        let errno = (bytes[0] << 24) as i32 |
                                    (bytes[1] << 16) as i32 |
                                    (bytes[2] <<  8) as i32 |
                                    (bytes[3] <<  0) as i32;
  • so does the code at line 640
            fn fail(output: &mut file::FileDesc) -> ! {
                let errno = os::errno();
                let bytes = [
                    (errno << 24) as u8,
                    (errno << 16) as u8,
                    (errno <<  8) as u8,
                    (errno <<  0) as u8,
                ];
                assert!(output.inner_write(bytes).is_ok());
                unsafe { libc::_exit(1) }
            }
  • ./src/libstd/io/mod.rs:393 casts errno to an i32
        fn get_err(errno: i32) -> (IoErrorKind, &'static str) {
            // FIXME: this should probably be a bit more descriptive...
            match errno {
                libc::EOF => (EndOfFile, "end of file"),
                <clipped>
            }
        }

        let (kind, desc) = get_err(errno as i32);
        IoError {
            kind: kind,
            desc: desc,
            detail: if detail && kind == OtherIoError {
                Some(os::error_string(errno).as_slice().chars().map(|c| c.to_lowercase()).collect())
            } else {
                None
            },
        }
    }
  • IoError::from_errno signature will need to change of errno changes to a c_int

@tomjakubowski
Copy link
Contributor

cc #16786

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@donkopotamus
Copy link
Contributor Author

@alexcrichton I reopened this as PR #18642, which adds errno() -> c_int and set_errno(c_int) to libc and implements std::os::errno() -> int using those. This addresses some of the comments in this old PR.

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.

7 participants