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

Introduce set_errno #2283

Merged
merged 15 commits into from
Jan 8, 2024
Merged

Introduce set_errno #2283

merged 15 commits into from
Jan 8, 2024

Conversation

PolyMeilex
Copy link
Contributor

@PolyMeilex PolyMeilex commented Jan 6, 2024

What does this PR do

This adds a way to set errno, it makes it possible to fully drop errno crate in crates that already use nix.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@PolyMeilex PolyMeilex marked this pull request as ready for review January 6, 2024 21:31
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

How about a usage example? And FTR, what function from the errno crate does this replace? Oh, and don't worry about the test failure on DragonflyBSD; it's unrelated.

@PolyMeilex
Copy link
Contributor Author

PolyMeilex commented Jan 6, 2024

How about a usage example?

If you are asking for a use case, I simply noticed that fish shell depends both on nix and errno, so I digged into it and that's how I found out that nix does not have a way to set errnos.
As to why they need a way to set errno, the project is transitioning from C++ so errno is being used here and there.
But same need would arise if one is writing rust libs meant for consumption from C.

And if you are asking me to add rustdoc usage example, sure I can do that, just let me know that I misunderstood the question.

And FTR, what function from the errno crate does this replace?

Simply errno::set_errno

@asomers
Copy link
Member

asomers commented Jan 6, 2024

Yeah, a rustdoc example would be great. Also, you'll have to rebase to fix the CI failure.

@PolyMeilex PolyMeilex force-pushed the set-errno branch 2 times, most recently from 95c18b4 to 47f3223 Compare January 7, 2024 00:13
@PolyMeilex
Copy link
Contributor Author

Rebased, examples added to module docs, and to set and clear functions

@PolyMeilex
Copy link
Contributor Author

I'm not sure if this should be top level function, if clear is a static method already?

Currently we have:

pub fn errno() -> i32;
pub fn set_errno(errno: Errno);

impl Errno {
    pub fn clear(); // Does the same'ish action as set but lives in diferent scope
    pub fn last() -> Errno;
}

So that's not consistent.

But moving set to a method does not seem entirely right either?

impl Errno {
    pub fn set(&self); // Like so?
    pub fn set(errno: Errno) // Or perhaps like this?
}

Personally I would just move clear to top level as well to make it consistent, but that's already part of public API, so I would rather not break it for no reason.

@asomers
Copy link
Member

asomers commented Jan 7, 2024

Excellent question. I agree that the current situation is inconsistent. I'm a fan of namespaces, so my preference would be:

impl Errno {
    fn set(self) {...}
    fn clear() {...}
    fn last() { ...}
}

@SteveLauC
Copy link
Member

SteveLauC commented Jan 7, 2024

I'm not sure if this should be top level function, if clear is a static method already?

Currently we have:

pub fn errno() -> i32;
pub fn set_errno(errno: Errno);

impl Errno {
    pub fn clear(); // Does the same'ish action as set but lives in diferent scope
    pub fn last() -> Errno;
}

So that's not consistent.

It is indeed not consistent.

I think the following approach is ok:

impl Errno {
    pub fn set(errno: Errno) {}

So when people use it, Errno:: would just behave like a namespace.


For nix::errno::errno() -> i32, Maybe we should also move it to:

impl Errno {
    /// Returns the platform-specific value of errno
    pub fn last_raw() -> i32 { }
}

And nix::errno::from_i32() (consts::from_i32()) should be marked pub(crate) so that we don't expose 2 interfaces for 1 thing.

If we have last_raw(), then this function should probably be re-named as from_raw()

@asomers thoughts on this?

@asomers
Copy link
Member

asomers commented Jan 7, 2024

I agree with renaming errno() to Errno::last_raw. But what exactly do you want to rename from_raw, and why?

@SteveLauC
Copy link
Member

I agree with renaming errno() to Errno::last_raw. But what exactly do you want to rename from_raw, and why?

Just for symmetry:

impl Errno {
    pub fn from_raw(raw: i32) -> Errno
    pub fn last_raw() -> i32
}

@SteveLauC
Copy link
Member

SteveLauC commented Jan 7, 2024

To deprecate an interface, we do it like this.

The version of the next release will be 0.28.0.

@PolyMeilex
Copy link
Contributor Author

PolyMeilex commented Jan 7, 2024

To deprecate an interface, we do it like this.

The version of the next release will be 0.28.0.

Done.

Although I have no idea why the CI is failing, I can't figure out where is the code that causes this error:
EDIT: Nevermind, just had to rebase

@SteveLauC
Copy link
Member

I agree with renaming errno() to Errno::last_raw. But what exactly do you want to rename from_raw, and why?

Just for symmetry:

impl Errno {
    pub fn from_raw(raw: i32) -> Errno
    pub fn last_raw() -> i32
}

LGTM! Thanks!

But I would like to know @asomers's view on the new APIs before merging this PR.

src/errno.rs Outdated Show resolved Hide resolved
src/errno.rs Outdated Show resolved Hide resolved
test/test_errno.rs Outdated Show resolved Hide resolved
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteveLauC SteveLauC added this pull request to the merge queue Jan 8, 2024
Merged via the queue into nix-rust:master with commit c505277 Jan 8, 2024
35 checks passed
@PolyMeilex PolyMeilex deleted the set-errno branch January 8, 2024 11:29
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.

3 participants