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 recv to use correct values on FreeBSD #343

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

180watt
Copy link
Contributor

@180watt 180watt commented Jul 28, 2024

It's rather unfortunate that Rust's libc doesn't know that FreeBSD has POLLRDHUP, but in the mean time we can add the value ourselves. We also need to use a uint for nfds_t. It seems that illumos also has POLLRDHUP, so this implementation could work there too. (Testing welcome.) I'm a novice when it comes to Rust, so any suggestions to this patch is very welcomed.

P.S. I failed to mention this, but in its current state (pre-patch), ipc-channel fails to build on FreeBSD

@wusyong
Copy link
Member

wusyong commented Jul 29, 2024

Can you either change only the parts that are affected, like POLLRDHUP and poll, or add a comment for the method to describe the difference between Linux and FreeBSD?

@180watt
Copy link
Contributor Author

180watt commented Jul 29, 2024

Updated my commit to be more "surgical." Thanks for the suggestions.

@180watt 180watt force-pushed the main branch 2 times, most recently from 0a84f22 to 8535ae0 Compare July 29, 2024 06:22
#[cfg(target_os = "freebsd")]
let result = libc::poll(
fd.as_mut_ptr(),
fd.len() as libc::c_uint,
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to use as _ and avoid any platform-specific code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed up.

Use correct value for POLLRDHUP and make fd.len cast more portable.

Signed-off-by: 180watt <180watt@duck.com>
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Thanks!

@jdm jdm enabled auto-merge July 29, 2024 14:05
@jdm jdm added this pull request to the merge queue Jul 29, 2024
Merged via the queue into servo:main with commit da28df9 Jul 29, 2024
12 of 16 checks passed
@180watt
Copy link
Contributor Author

180watt commented Jul 29, 2024

I have missed a critical c in the code, and we are now trying to do lib::c_short. Of course this had to be in the path that I didn't build, so I didn't see the error. I am very sorry about this. I am also currently away from my computer and can not readily push something. Do you mind either reverting or pushing a fix?

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