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

feat: make compile pass on windows #238

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

loongs-zhang
Copy link
Contributor

@loongs-zhang loongs-zhang commented Feb 6, 2024

Simply supporting compilation on Windows is already a huge workload. This PR solves most compilation errors, and I found that the buf module is missing msg. Do you plan to manually assemble libc::msghdr/windows_sys::Win32::Networking::WinSock::WSAMSG directly? @ihciah

@loongs-zhang loongs-zhang marked this pull request as draft February 7, 2024 02:14
@loongs-zhang loongs-zhang changed the title feat: implement write op on windows feat: make compile pass on windows Feb 7, 2024
@loongs-zhang loongs-zhang changed the title feat: make compile pass on windows feat: implement write op on windows Feb 7, 2024
@loongs-zhang loongs-zhang marked this pull request as ready for review February 7, 2024 03:03
@loongs-zhang loongs-zhang force-pushed the dev-windows-support branch 3 times, most recently from a30e577 to a4072ae Compare February 8, 2024 16:09
@loongs-zhang
Copy link
Contributor Author

@CarrotzRule123 Could you please help me?

@loongs-zhang loongs-zhang force-pushed the dev-windows-support branch 3 times, most recently from 09897ba to 5424253 Compare February 9, 2024 14:48
@loongs-zhang loongs-zhang changed the title feat: implement write op on windows feat: make compile pass on windows Feb 9, 2024
@ihciah
Copy link
Member

ihciah commented Feb 19, 2024

Thank you so much! Supporting windows requires a lot of work.
I have little background knowledge about winapi, can you clarify the

manually assemble libc::msghdr/windows_sys::Win32::Networking::WinSock::WSAMSG directly

?

@loongs-zhang
Copy link
Contributor Author

loongs-zhang commented Feb 19, 2024

For example, in monoio/src/driver/op/send.rs:103

impl<T: IoBuf> Op<SendMsg<T>> {
    pub(crate) fn send_msg(
        fd: SharedFd,
        buf: T,
        socket_addr: Option<SocketAddr>,
    ) -> io::Result<Self> {

// manually assemble the libc::msghdr
// I think we should add `msg` submodule for `buf`

        let iovec = [libc::iovec {
            iov_base: buf.read_ptr() as *const _ as *mut _,
            iov_len: buf.bytes_init(),
        }];
        let mut info: Box<(Option<SockAddr>, [libc::iovec; 1], libc::msghdr)> =
            Box::new((socket_addr.map(Into::into), iovec, unsafe {
                std::mem::zeroed()
            }));

        info.2.msg_iov = info.1.as_mut_ptr();
        info.2.msg_iovlen = 1;

        match info.0.as_ref() {
            Some(socket_addr) => {
                info.2.msg_name = socket_addr.as_ptr() as *mut libc::c_void;
                info.2.msg_namelen = socket_addr.len();
            }
            None => {
                info.2.msg_name = std::ptr::null_mut();
                info.2.msg_namelen = 0;
            }
        }

        Op::submit_with(SendMsg { fd, buf, info })
    }
}

I also have some doubts about SendMsg and SendMsgUnix. It seems that SendMsgUnix should be used in #[cfg(unix)], and SendMsg should be used in #[cfg(windows)] ?

@ihciah
Copy link
Member

ihciah commented Feb 19, 2024

It seems that SendMsgUnix should be used in #[cfg(unix)]

Yes, you are right. It is used by unix datagram and seq_packet, so it is unix only.

Manually construct msghdr should be ok. Maybe WSAMSG should be ok too since it is a repr(C) struct? Our first step is to try to get it work, so if it can work correctly, it is ok for now:)

@loongs-zhang
Copy link
Contributor Author

Got it, so RecvMsgUnix also has a similar problem.

I plan to use SendMsgUnix/RecvMsgUnix as an alias for SendMsg/RecvMsg to ensure good compatibility.

Then refer to the implementation of monoio/src/driver/op/write.rs:17 to finish SendMsg&RecvMsg in the next PR.

Any other suggestions? @ihciah

@ihciah
Copy link
Member

ihciah commented Feb 19, 2024

use SendMsgUnix/RecvMsgUnix as an alias for SendMsg/RecvMsg

For linux both SendMsgUnix and SendMsg may be used at the same time. I suggest to add cfg unix to SendMsgUnix/RecvMsgUnix and the code which only related to unix.

@loongs-zhang
Copy link
Contributor Author

Sure, I see the differences between the structures.

Copy link
Member

@ihciah ihciah left a comment

Choose a reason for hiding this comment

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

It's a great work, thank you!

@ihciah ihciah merged commit 0f8b724 into bytedance:master Feb 19, 2024
77 of 79 checks passed
@loongs-zhang loongs-zhang deleted the dev-windows-support branch February 19, 2024 08:55
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.

None yet

2 participants