search

tokio-rs / io-uring

The `io_uring` library for Rust
Apache License 2.0
1.16k stars 129 forks source link

Update documentation for `ReadFixed` and `WriteFixed` #276

Closed connortsui20 closed 4 months ago

connortsui20 commented 4 months ago

Adds some detail to the ReadFixed and WriteFixed opcodes (referred to in #275).

There is one small problem with this: I've documented the expected behavior according to the man pages, but I have actually yet to see these opcodes work in Rust. Are there any tests that check if these opcodes are correct? (My personal testing seems to not work, but it is very possible I've just set it up wrong since I haven't looked too deeply into it...)

Also, I am not completely sure why the buf_index is a u16 type rather than a i32. It seems like the man pages for io_uring_prep_write_fixex allow for a 32-bit buffer index, and I don't think that there is a 16-bit limit on the number of buffers we are allowed to register (see the man page for io_uring_register_buffers, we are allowed to register an unsigned number of buffers).

I don't want to make this PR more than it should be, so I am happy to create another issue for the 2 things I mentioned.

quininer commented 4 months ago

https://github.com/axboe/liburing/blob/liburing-2.5/src/include/liburing.h#L466 https://github.com/tokio-rs/io-uring/blob/master/src/sys/sys.rs#L743

buf_index is u16 because buf_index in entry can only be u16.

quininer commented 4 months ago

I don't want docs to become too complex because I don't have the energy to track and maintain too much docs. It would be nice if you just fixed typos, or were willing to take on the maintenance of docs.

connortsui20 commented 4 months ago

@quininer Honestly, I think I would be willing to take on maintenance of the docs. Though I have never done that before and also I have never actually used the raw C interface of io_uring and am only familiar with liburing, so out of caution I probably shouldn't take on such a large task that I'm not 100% familiar with.

As for this specific PR, if you are okay with just linking to the liburing page I can remove some extra docs.

FrankReh commented 4 months ago

Yes, I always found the liburing man pages to be the best source for io_uring details. And the small group that manages them also is responsible for the io_uring kernel driver directly so any questions about their man pages and their header files are always quickly answered and they take PRs against their man pages very well. If the documentation here doesn't point to those man pages, it probably should.

connortsui20 commented 4 months ago

@quininer I removed the extra docs so that now it just points to the lib_uring man pages, let me know if you think this is sufficient!

FrankReh commented 4 months ago

@Connortsui20 I guess I didn't mean I thought it would help if individual opcode documentation in this repo pointed to the liburing man pages. Just that the whole usage of io-uring had to be understood by looking at the liburing man pages for details of the kernel API and then how the liburing headers worked off of that kernel API.

The liburing man pages don't try to describe all the details. They usually refer back to the original level 2 or level 3 manpage of the original kernel API.

Likewise this repo, I think, is taking the reasonable position that it would be too much to document details of each opcode. The user of this repo should just be educated somewhere at a high level for all its opcodes, that a clearer understanding can be gotten by reading through the liburing man pages and the liburing source, and sometimes the liburing unit test source.

The documentation here is a different level than that of liburing, but that's reasonable. It's remarkable that FaceTime affords the time to the authors to document and test as much as they do, for an open source project. Many projects, it seems, like having this repo for building against, and they undoubtedly dig into the details of how to make their use of the APIs work for them.

connortsui20 commented 4 months ago

@FrankReh Yes that makes sense. However I don't think it's unreasonable for opcodes to point to specific man pages. Just because "most" people using this library should be educated on the high level usage does not mean this should purposefully exclude useful information. And additionally, educated people can forget where things are.

@quininer If you think that pointing to the man page is too much I'll remove it and open a new issue for discussion later.

quininer commented 4 months ago

I agree with @FrankReh

connortsui20 commented 4 months ago

@quininer Done

quininer commented 4 months ago

Thank you!