-
Notifications
You must be signed in to change notification settings - Fork 675
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
fix #480 and add simple test cases for that. #486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just one question and one change request. Looks good otherwise.
Could you rebase to master, before pushing again (with -f), so that the CI will not expect the 1.7 builds to work.
pub fn epoll_ctl<'a, T>(epfd: RawFd, op: EpollOp, fd: RawFd, event: T) -> Result<()> | ||
where T: Into<&'a mut EpollEvent> | ||
{ | ||
let res = unsafe { libc::epoll_ctl(epfd, op as c_int, fd, &mut event.into().event) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for this to be safe, we should check, if op
is EPOLL_CTL_DEL
when event
is NONE. Otherwise we might pass a null pointer into libc::epoll_ctl
, when it expects something else.
EpollEvent { event: libc::epoll_event { events: events.bits(), u64: data } } | ||
} | ||
|
||
pub fn empty() -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give me an example of a use case for this function?
EpollEvent::empty() is a try for simplify this case:
Do you think that's a good idea? Maybe I should move that to another issue for more deep discussion. As for another question, we should return an error when call |
Sorry for forgeting to edit CHANGLOG. If all design are reviewed and OK, I will complete that. |
I would return |
Got it. I will fix all and make pull request again. Thank you! |
I think everything is OK. Please check it again. Thanks! |
Anything wrong? |
No. I have not looked into this project for a while. Sorry. @homu r=fiveop |
📌 Commit acc1513 has been approved by |
☀️ Test successful - status |
r? @fiveop