search

ringbahn / uring-sys

liburing bindings
Apache License 2.0
33 stars 14 forks source link

Add io_uring system calls #6

Closed twissel closed 4 years ago

twissel commented 4 years ago

I not sure about __NR* naming...

withoutboats commented 4 years ago

These wrappers around the syscalls are defined in syscall.c, which we are already compiling (and which the C code we link to uses). I'd be inclined to re-expose the definition from liburing instead of writing our own on top of libc::syscall (even though they're currently trivial functions). In general I'd like to have no functions with bodies in uring-sys, just type, const and fn definitions that let users bind to liburing's code.

Do you have a reason you implemented them in Rust instead of declaring them against the extern blocks?

As to publicly exposing the syscall constants and what naming convention to use, I'm not certain. I'd like to see what other libraries have done in Rust follow their lead. I lean a bit toward not to expose them at all.

twissel commented 4 years ago

I'm fully agree with your comment, I'm just forgot that syscalls already defined in liburing, was tired, sorry :(

twissel commented 4 years ago

Done, really hope I didn't screw up somewhere

twissel commented 4 years ago

Well, it seems that now liburing is not exporting this system calls https://github.com/axboe/liburing/commit/96144ea798b15e41f97270383fc15e5c8aec893f, https://github.com/axboe/liburing/commit/39ece4a8e2f88352976e1240d466df077a68a9b9

withoutboats commented 4 years ago

That's interesting. Not sure what we should do. It also makes me concerned that liburing isn't as committed to backwards compatibility as I assumed.

withoutboats commented 4 years ago

Commit message with more information:

Rename the iouring system calls to __sys_iouring and make them locals.

We only have the system calls because they are not in glibc yet, and it's somewhat confusing that they share the same namespace as the library functions. With this change, any exported iouring* function is a library function.

Not sure if we should implement these ourselves here or not. This is supposed to be bindings to the liburing library. Does it make sense to couple exposing an API for the syscalls to exposing an API for the related C library, or should there instead be another crate that just exposes these three syscalls in Rust?

twissel commented 4 years ago

I'd rather implement them in uring-sys

withoutboats commented 4 years ago

Yea that sounds fine, and its not like the shims will be very hard to maintain.

I would propose going back to implementing them through libc::syscall but exposing them in a submodule syscalls.rs, to separate from the liburing part of the API.

twissel commented 4 years ago

Attempt № 4 :)

withoutboats commented 4 years ago

Looks great! Thanks!