ringbahn / iou

Rust interface to io_uring
Apache License 2.0
328 stars 22 forks source link

sqe "poll" methods #26

Closed twissel closed 4 years ago

withoutboats commented 4 years ago

Thanks for the PR! Sorry, I'll be moving slow for a bit on reviewing because I'm quite busy with non-Rust stuff right now.

Overall this looks good, thanks for writing a test as well. My main question is one you already raised on the related issue: should we define our own PollMask type or should we start depending on nix and use theirs.

nix only adds two really small dependencies other than itself: cfg-if and void. But nix itself is a pretty large dependency, covering a lot of different unix APIs. I wonder: are there other types we would want to pull from nix for other prep methods, or would it just be for the poll flags?

twissel commented 4 years ago

If i'm not wrong we can use: 1) https://docs.rs/nix/0.16.0/nix/sys/socket/enum.SockAddr.html for https://docs.rs/uring-sys/1.0.0-beta/uring_sys/fn.io_uring_prep_connect.html 2) https://docs.rs/nix/0.16.0/nix/sys/socket/struct.RecvMsg.html for https://docs.rs/uring-sys/1.0.0-beta/uring_sys/fn.io_uring_prep_recvmsg.html 3) https://docs.rs/uring-sys/1.0.0-beta/uring_sys/fn.io_uring_prep_accept.html is odd because:

The addrlen argument is a value-result argument: the caller must initialize it to contain the size (in bytes) of the structure pointed to by addr; on return it will contain the actual size of the peer address.

4) And they have useful enums like PollMask, MsgFlags, etc

withoutboats commented 4 years ago

Well that's certainly a lot of APIs so I think depending on nix makes sense. The behavior of accept is tricky since we would normally want to use a wide pointer with a length instead of a separate length.

twissel commented 4 years ago

For now we can just omit this params, and pass null instead, like nix does https://docs.rs/nix/0.16.0/src/nix/sys/socket/mod.rs.html#971

withoutboats commented 4 years ago

Thanks for the PR! Sorry it took me so long to review, I've been moving between countries and then needed laptop repair but now have a Linux machine again. :tada:

Going to merge this as is, but I'll open an issue to depend on nix instead of defining PollMask before we make a new release.