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

Add libc::errno and libc::set_errno and use to implement std::os::errno #18642

Closed

Conversation

donkopotamus
Copy link
Contributor

This pull request does the following:

  • adds two functions libc::errno() -> c_int and libc::set_errno(c_int)
  • implements std::os::errno using libc::errno
  • removes the currently disparate signatures for std::os::errno(), which returns int on unix and uint on windows, in favour of returning int only.

This represents a reopening of previously discussed PR #17265.

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

}
}

_errno_ptr()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what was the benefit of defining errno_ptr() on top of _errno_ptr() which can hide platform-dependency by themselves.

Incidentally, ::{c_int} isn't beautiful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question of whether use <...>::{c_int} is not beautiful is probably a debatable one :-). In either case, the style has been chosen as it matches all of the other single name use ... statements within this source file.

As to defining errno_ptr() over _errno_ptr(). I guess the benefits are that it ensures errno_ptr is implemented on all target_os possibilities. If a value of target_os is not covered by the _errno_ptr implementations, the function errno_ptr won't compile, providing a hint that it needs to be implemented for that new platform. (granted, if it wasn't done this way, errno and set_errno wouldn't compile, but that is a slightly less specific error)

It also means the function only needs to be documented at this single entrypoint.

But again, the implementation was mostly influenced by what the existing errno source had been doing.

@nodakai
Copy link
Contributor

nodakai commented Nov 9, 2014

Can you also update

pub fn from_errno(errno: uint, detail: bool) -> IoError {

Also, I wonder if adding sth like std::os::clear_errno() can be useful because what we set to errno is 0 for 99 % of time. This is just a random thought...

@donkopotamus
Copy link
Contributor Author

@nodakai Yes, IoError::from_errno can be changed now that std::os::errno is consistent about returning only an int (rather than the previously mix of int vs uint depending on platform).

I hadn't initially put in to this PR as I also think there are a few other cleanups required (see my next to last comment in old PR #17265)

@alexcrichton
Copy link
Member

Right now liblibc is in theory purely just declarations of C functions available on the host system rather than providing common abstractions and implementations. Now that we have std::sys, this seems like it would be appropriately placed there instead. Currently the entire sys module is private, but we plan on making it public over time once we hammer out APIs and where functions are expected to go.

@donkopotamus
Copy link
Contributor Author

@alexcrichton Presume you mean that std::os::errno() (and possibly clear_errno()) should eventually shift to std::sys or are you referring to the libc::errno() and lib::set_errno()? The latter reflect c95 and would seem well placed in libc

@thestinger
Copy link
Contributor

I don't think wrappers that aren't defined by the C standard library belong in libc.

@alexcrichton
Copy link
Member

@donkopotamus oh I'm sorry this got lost in my inbox! To answer your question, I was referring to libc::errno. As @thestinger said, the liblibc library is basically just a library for purely bindings to libc functions, not for function declarations themselves.

@donkopotamus
Copy link
Contributor Author

@alexcrichton I can understand that argument ... errno is a slightly odd case in ISO C I guess in that its a modifiable lvalue rather than a function ... so it can't actually sit in liblibc without a rust wrapper of some kind.

I moved it into libc in response to the commentary on the previous pull request which seemed to get bogged down around whether errno should return c_int vs int. As a function, errno() does seem to be principally used for detected errors in C functions (naturally) meaning the libraries are sprinkled with snippets such as if os::errno() != libc::ERROR_IO_PENDING as uint etc.

For myself, I'm just interested in having set_errno() defined somewhere!

@alexcrichton
Copy link
Member

@donkopotamus I agree that it's not so great right now, but currently liblibc is just a wrapper crate with no extra code in it (as much as possible). The story for this will become much clearer over time as we flesh out the story for std::sys, I would expect however.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with additions to std::os::{windows, unix}!

lnicola added a commit to lnicola/rust that referenced this pull request Dec 11, 2024
minor: Remove unstable attributes in minicore
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.

5 participants