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

NetBSD: add two more errno values #3040

Merged
merged 2 commits into from
Apr 22, 2023
Merged

NetBSD: add two more errno values #3040

merged 2 commits into from
Apr 22, 2023

Conversation

0-wiz-0
Copy link
Contributor

@0-wiz-0 0-wiz-0 commented Dec 18, 2022

available in NetBSD-current since 2020

available in NetBSD-current since 2020
@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information.

@workingjubilee
Copy link
Member

Why would this be a "breakage candidate"? Because it changes ELAST? ELAST on OS that use this convention is defined as being the largest errno. It's only intended to be used to allow checks to see if an error is within the known values for errno, and things of that nature, it's not really directly used by any API. It is "versioning in userspace", as it were.

@JohnTitor
Copy link
Member

Why would this be a "breakage candidate"? Because it changes ELAST? ELAST on OS that use this convention is defined as being the largest errno. It's only intended to be used to allow checks to see if an error is within the known values for errno, and things of that nature, it's not really directly used by any API. It is "versioning in userspace", as it were.

Yes, and I think it could cause a bug (or any weird behavior due to lack of errno) if the last value points to a newer number while the user env is older.
As you mentioned on #3131, we just should not provide such a const, I think.

@workingjubilee
Copy link
Member

Hmm. I think that would be due to invalid reasoning on the part of the programmer, which we can't completely protect people from, but I agree that it's in any case quite silly from the Rust PoV.

One option:

  • Merge this change with the updated ELAST value, but also with a deprecation notice. People who are using ELAST "correctly" don't have to change anything immediately, so it doesn't introduce bugs in code that was written to be future-proof, but they can use the deprecation notice to prepare. People who were using it incorrectly have a heads up.

I think in this case having an errno with a larger value than ELAST is equally likely to introduce bugs, unfortunately.

@workingjubilee
Copy link
Member

Noting that #1902 was handled by leaving the value in place and deprecating it, though that time we said we'd change it.

@JohnTitor
Copy link
Member

@0-wiz-0 Could you apply this?

  • Merge this change with the updated ELAST value, but also with a deprecation notice. People who are using ELAST "correctly" don't have to change anything immediately, so it doesn't introduce bugs in code that was written to be future-proof, but they can use the deprecation notice to prepare. People who were using it incorrectly have a heads up.

@rustbot author

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented Apr 21, 2023

@JohnTitor I've added a deprecation warning as requested. Did I do it correctly?

@JohnTitor
Copy link
Member

Yeah, thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Apr 22, 2023

📌 Commit 06fdfac has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 22, 2023

⌛ Testing commit 06fdfac with merge 9e90211...

@bors
Copy link
Contributor

bors commented Apr 22, 2023

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 9e90211 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants