-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Refactoring the Linux module: first try. #373
Refactoring the Linux module: first try. #373
Conversation
…r fix must be found.
(rust_highfive has picked a reviewer for you, use r? to override) |
I haven't looked carefully, but "reuse more code" and "+760 −316" diff look a bit contradicting, no? Is some new stuff added? |
Yes, there is indeed some added stuff. And a part of it are comments. |
So, I finally managed to put most of the bugs out. There are only three remaining, for which I have no clue. So, asking for help. On On On I don’t really understand how the tests are done so, maybe I’m missing something, or maybe the tests are wrong somehow, I don’t know. I would really use some insight here. |
Thanks for the PR @GuillaumeLestringant! Before going much farther, though, I think we may want to get on the same page about the motivation of this refactoring. Could you help walk me through what you're thinking? Today this crate optimizes for readability rather than writing new bindings themselves. That is, we greatly favor duplication over a jungle of Finally, the duplication is all validated through the CI infrastructure on this repository. That is, basically all definitions/constants/functions are automatically verified, so there's no risk of duplication silently introducing bugs. So given all that, could you walk me through the high level goals and rationale of this refactor? Additionally, I'm curious where this'll all end up eventually! |
☔ The latest upstream changes (presumably #377) made this pull request unmergeable. Please resolve the merge conflicts. |
So this is a first try of the refactoring proposed in #371. It sets the tree structure up, and gives a go at the
fcntl.h
header. It is an interesting choice because it has type and structure definitions, generic and platform specific macros, and also POSIX and Linux specific definitions, as well as glibc and musl specific ones. So it covers more or less all possible cases.For clarity’s sake, all definitions of this header have been placed at the end of their respective sections, along with a comment saying:
/* Header <fcntl.h> */
Doing so with all headers should make sure that a specific definition is easier to find than in the mess we have for now.
A few notes on unexpected issues.
gnu
submodule underother
(and auclibc
one, that remains empty for now).file_handle
whose last element is a zero-sized array. This is rendered by a dynamically-sized array in Rust, but then, the struct cannot implement theCopy
trait, and makes thes!
macro bug. I commented that line out, but a better solution would be wished.file_offset64
that should be equivalent to__USE_FILE_OFFSET64
. This feature must be defined in musl targets (!), but I don’t know how to make sure it is the case. As a palliative, I added the conditiontarget_env = "musl"
everywherefile_offset64
is used, but this cannot be definitive. Advice strongly wished here.PS: I can’t test every platform at home, so there might be some remaining bugs. Will add some commits when I know where they are.