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

update epoll constants to match epoll_event struct #3466

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

tremon015
Copy link

Change the type of the EPOLL* event constants to match the type of the events field in struct epoll_event (i.e. u32).

closes: #3462

Change the type of the EPOLL* event constants to match the type of
the events field in struct epoll_event (i.e. u32).

closes: rust-lang#3462
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

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. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@tremon015
Copy link
Author

This should probably be considered a breaking change, but I'm unsure how to address that properly. I don't think deprecating the current constants and introducing new ones to reflect the type change is in any way a proper solution. Anyone using these constants in the context of epoll_event will already have these values wrapped as (constant as u32), so for these cases this change will not cause any breakage. However, I'm not convinced there are no other uses for these constants outside of the epoll_event context.

@devnexen
Copy link
Contributor

devnexen commented Dec 8, 2023

Could be it or creating a type alias from it __poll_t (especially, as you mentioned, it s tagged with bitwise attribute).

@tremon015
Copy link
Author

tremon015 commented Dec 8, 2023

I guess that depends on whether we want kernel types to be exposed directly through the libc crate. For as far as I can see, the __poll_t type is part of the Linux syscall interface, but not the glibc wrapper. The type is exposed via <linux/types.h> so it is available to userspace, but the glibc header file <sys/epoll.h> does not use the __poll_t type at all, it just redefines the values in an enum:

enum EPOLL_EVENTS
  {
    EPOLLIN = 0x001,
[..]
    EPOLLET = 1u << 31
  };

What would be the rust equivalent of the bitwise attribute?

@devnexen
Copy link
Contributor

devnexen commented Dec 8, 2023

you re right then u32 seems good enough as is.

Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Seems safe enough, and it is unlikely existing code will break, it may get warnings though which will be easy to fix.

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2024

📌 Commit 9a40025 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 7, 2024

⌛ Testing commit 9a40025 with merge 77fb037...

@bors
Copy link
Contributor

bors commented Jan 7, 2024

☀️ Test successful - checks-actions, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 77fb037 to main...

@bors bors merged commit 77fb037 into rust-lang:main Jan 7, 2024
11 checks passed
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.

epoll event constants should be declared as u32
6 participants