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 documentation for ReadFixed and WriteFixed #276

Merged
merged 4 commits into from
May 14, 2024

Conversation

connortsui20
Copy link
Contributor

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.

src/opcode.rs Outdated Show resolved Hide resolved
src/opcode.rs Outdated Show resolved Hide resolved
src/opcode.rs Outdated Show resolved Hide resolved
@quininer
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

FrankReh commented May 6, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Member

I agree with @FrankReh

@connortsui20
Copy link
Contributor Author

@quininer Done

@quininer quininer merged commit fe2eade into tokio-rs:master May 14, 2024
12 checks passed
@quininer
Copy link
Member

Thank you!

@connortsui20 connortsui20 deleted the opcode-fixed-doc-update branch May 14, 2024 02:37
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