ioctl work#147
Conversation
This is more type-safe. Also, the old code wasn't cross-platform at all even though it claimed to be. It wasn't even portable across architectures on Linux.
|
cc @ltratt @utkarshkukreti @posborne @kennytm @fhartwig, people who have touched the previous ioctl implementation. |
|
Wow, some of the tests are really volatile.. |
|
Yeah, not sure about the tcgetattr test. For the waitpid one, setting RUST_TEST_THREADS=1 should disable all parallellism I think? |
|
@cmr, yes RUST_TEST_THREADS=1 should work but it seems heavy handed. Only the one wait() test need to be serialized. |
|
@cmr, ioctls's are "fun" aren't they? I took the last shot at ioctl. Initially, I think this implementation should cover the use cases I have had for library like https://github.com/posborne/rust-spidev while providing better cross-platform support. The macros for |
|
Fortunately, the "bad" is only exposed internally. I have no particular feelings about the syntax that the |
|
Given this doesn't seem to touch the OpenBSD parts I added, I don't have any useful comments :) |
|
cc @posborne. Hoping to get some feedback (given my lack of ioctl knowledge) |
|
@ltratt Note that this patch removes support for all other platforms but Linux, as I haven't personally looked into their command encoding etc. |
I really feel that this is an unrealistic expectation based on the real work I have done with ioctls in the past, particularly on embedded Linux systems where you end up talking to a larger variety of devices than one might see in a desktop or server environment. I think we need to leave the door open for users and library providers to be able to define their own ioctls. The question for me is whether it is even worth trying to "automatically" provide any kind of interface for the specific ioctls available on any given system. Providing an automatic interface that provides a good and safe interface to most ioctls is, in my opinion, a losing battle (which is why new drivers are not accepted using ioctl in Linux). I would rather not provide methods for accessing system interfaces than to provide ones that are broken. I'm working to see if I can port my SPI library to using the new interface as a trial. Do you have example code using the interface that you can show as well? |
There was a problem hiding this comment.
The previous implementation returned a nix::Result with Ok for res >= 0. 0 is typical for a return value but a length is sometimes returned that is greater than 0[1]. The error returned also handled getting error information from the errno.
Unless there is a good reason to do else-wise, I think we would want to do the same here.
There was a problem hiding this comment.
That's an excellent idea.
There was a problem hiding this comment.
The old interface also was provided ioctl::read with the following signature:
pub unsafe fn read<T>(fd: RawFd, op: ioctl_op_t) -> Result<T> {
...
}Whereas the current interface provides the somewhat less safe and friendly interface (abbreviated for clarity):
(read $name:ident with $ioty:expr, $nr:expr; $ty:ty) => (
pub unsafe fn $name(fd: c_int, val: *mut $ty) -> c_int {
...
}
);The main difference being that the user must always pass in a *mut ty in the new implementation where they would just get back an owned value in the new implementation. I see no difference in safety between the two implementations (both are unsafe) but there is more the user could screw up in the new interface. The old implementation also provided read_into<T>(fd: RawFd, op: ioctl_op_t, data: &mut T) -> Result<c_int> and read_into_ptr<T>(fd: RawFd, op: ioctl_op_t, data_ptr: *mut T) -> Result<c_int> as they are useful in some cases. The first mentioned, however, is the most ergonomic for the cases in which it can be used.
If it's in a header, the scripts in
I agree -- does this implementation try to do that? In particular, nothing is safe. |
|
@posborne I wrote the original library this impl comes from for use in https://github.com/cmr/evdev |
I didn't spot anything obviously wrong while looking over the ioctls that were present (versus the ones that are left commented) that was obviously problematic. At the same time, most of the really interesting ioctls require read/writing to a structure which cannot be handled currently (would probably require something like bindgen for the structures, but that can get very messy). For evdev, it looks like you added the structure definitions for those structures that are part of the kernel API to nix itself. I think these (and the ioctl constants) should probably live with your library outside of nix (Awesome lib BTW; I may find opportunity to use that myself sometime soon). |
|
@cmr also, what was the reason for removing the debug impls for the various epoll types? |
|
I personally have no opinion about this, so I can merge once there is consensus that it is good to go! 😄 |
|
@carllerche bitflags now provides the |
|
@posborne I'd prefer it if |
|
@posborne ping, what are your thoughts re: this? |
|
Sorry for the slow response.
I think nix might be able to support all major ways of accessing kernel APIs (e.g. ioctl, nl80211, sysfs, functionfs) and some APIs that are commonly desired, but supporting all APIs seems overly ambitious and likely to make maintenance of the library difficult. There is real complexity in many of the APIs and capturing this for all APIs would involve a lot of code.
I think it is easier to maintain several crates, each with a specific purpose, than to maintain a monolithic crate that tries to do too much. With rust, we have a great packaging and distribution system in cargo and crates.io. For SPI devices, users can use the spidev crate I have created. For evdev access, they can use the evdev crate. |
|
@posborne While I agree that that is certainly the case for higher-level interfaces, the |
|
I've removed the contentious code. |
|
@posborne So, removing the various wrapper functions leaves this whole implementation in a precarious position. The macros make less sense when you're not generating all of the code with macros. I think I'd rather just maintain my |
|
@cmr, I do think the macros make it easier for describing ioctl APIs, even if we do not include generated or non-generated ioctl APIs in nix from the get-go. I think it is important to have a moderately stable API from the start; I may take a stab at changing the API exposed as discussed in comments eariler. If so, I'll let you know where those changes are so you can pull them into this PR. |
|
Ok, I made some additional changes to return a result type rather than just returning the integer from the underlying call. In order to build, I had to rebase on master and make one other change. You should be able to cherry-pick my two commits (if deemed acceptable) onto your branch: https://github.com/posborne/nix-rust/commits/ioctl-rework Here's the relevant diff: posborne/nix-rust@eb837f8...posborne:ioctl-rework |
|
Goodness, sorry for the delay myself... I think what @posborne suggested (and implemented) is excellent, and if he's happy with it, I'm happy with it! |
|
I think I'm OK moving forward with what we ended up with. Since it breaks API compatibility with previous versions, this should be a version bump (with semantic versioning, I believe a bump to the next 0.x is appropriate). With my changes, the one part I think I may have wrong is with the following macro... posborne/nix-rust@eb837f8...posborne:ioctl-rework#diff-0f39576519037fe2d243b366759ea808R94 I think that needs to be in a mod or something to prevent polluting the namespace of the code that uses the macro? I feel like I've seen code that does that since I wrote this. |
|
@posborne Are you worried about the |
|
@cmr, That was my concern. If it's not a problem, excellent! Good one Rust! |
|
So, this PR is ready to go then? Let me know and I will merge. Also, it would probably be a good idea to bump the version as part of the PR. |
|
Zomg! Finally merged! 🎉 |
|
Excellent! I've been using this new stuff for a new lib[1] I am working on, so I'm glad to see it merged up. |
5 participants
No description provided.