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

Improve ioctl docs #670

Merged
merged 16 commits into from
Jul 21, 2017
Merged

Improve ioctl docs #670

merged 16 commits into from
Jul 21, 2017

Conversation

Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jul 11, 2017

Integrates suggestions and comments made in #641. Basically we hid a lot of the internal workings of things and also revised the docs to make it more clear the exact API that nix is exposing.

There is a small amount of code cleanup I did in the macros. I also fixed the bad version of the ioctl! macro and also added a bad none version for use with no-data, hardcoded-number ioctls.

Would appreciate any and all feedback. Please especially fetch this code locally and generate the pretty docs for it (cargo doc --no-deps --open) so you can see what our users will see.

Closes #641.
Closes #573.

cc @cmr @roblabla

@Susurrus
Copy link
Contributor Author

I need to see if the changes I made broke anything across all of our platforms and I also want to have a go at using this within my serialport-rs crate.

@posborne if you'd like to check that this refactoring doesn't break things for you in rust-spidev, that'd also be appreciated.

@Susurrus Susurrus force-pushed the ioctl_docs branch 3 times, most recently from bc1b486 to ae0b5e9 Compare July 12, 2017 05:48
@Susurrus
Copy link
Contributor Author

Alright, I removed a few more constants that were unnecessary and also cleaned out two ctypes that were exported in the root for some reason. The generated docs are looking better and better!

@Susurrus Susurrus changed the title WIP: Improve ioctl docs Improve ioctl docs Jul 12, 2017
@Susurrus
Copy link
Contributor Author

There is also no bad read version of the ioctl! macro, so the datatype is always a *mut, even if it's a reading ioctland should be*const`. A minor annoyance, but still annoying.

@Susurrus
Copy link
Contributor Author

And the datatype for bad is always a *u8 when it should really be a proper datatype.

//! # fn main() {}
//! ```
//!
//! More examples on using `ioctl!` can be found in the [rust-spidev crate](https://github.com/posborne/rust-spidev).
Copy link
Member

@posborne posborne Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, this has moved over to https://github.com/rust-embedded/rust-spidev, although the other link does redirect.

//! Here's a few examples of how that can work for SPI under Linux
//! from [rust-spidev](https://github.com/posborne/rust-spidev).
//! A simple `ioctl` is `SPI_IOC_RD_MODE`. This ioctl works with the SPI interface on Linux. This
//! specific `ioctl` reads the mode of the SPI mode (one of 4 values, so 2 bits each) as a `u8`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reads the mode of the SPI mode device

@Susurrus
Copy link
Contributor Author

@posborne Thanks for the review. both are now fixed in the latest revision.

@Susurrus
Copy link
Contributor Author

Alright, I think this is as much work as needs to happen to really clean this up. @roblabla and @cmr would appreciate any feedback from you guys on this.

@posborne
Copy link
Member

Yep, changes look good. I'll leave this open for now in case otherwise want to review. Otherwise r=me is fine.

@Susurrus
Copy link
Contributor Author

Thanks @posborne. I also wanted to get a downstream repo using ioctl! onboard to make sure these changes were sound. It looks like rust-spidev hasn't had any work in a long time, as in it's using some pretty old dependencies. Is it still used? If so, would you want to try to update it to use the revised ioctl! macro?

Copy link
Contributor

@roblabla roblabla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like those changes. Just a few nitpicks here and there.

Those changes are still compatible with my own library (bindings to Binder ioctls), and the docs look a lot better now IMO :)

//! be difficult.
//!
//! Historically `ioctl` numbers were arbitrary hard-coded values. This changed to a more-ordered
//! system where the ioctl numbers had various subcomponents:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has various subcomponents ? Also, AFAIK, this is actually linux-specific, and the change was made a loooong time ago (before 2.6).

Maybe say "In Linux, most ioctl numbers are made of various subcomponents" instead ?

Maybe add a link to http://elixir.free-electrons.com/linux/latest/source/Documentation/ioctl/ioctl-number.txt for more information ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(As it turns out, FreeBSD also has _IOR/_IOW, so I guess it got adopted by the wider unix community ? I can't find them referenced anywhere in POSIX though)

//!
//! The return value for `ioctl` functions generated by the `ioctl!` macro are assumed to return -1
//! on error and everything else is a valid return value. If the return value needs to be changed,
//! you can use `Result::map` on the return value in a helper function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe specify that when the ioctl function returns -1, we will use errno to build our Error value.

//! There are equivalent forms for `write`, `none` (no data in or out), and `readwrite`. The mode
//! for a given `ioctl` should be clear from the documentation if it has good documentation.
//! Otherwise it will be clear based on the macro used to generate the `ioctl` number where
//! `_IOC`, _IOR`, `_IOW`, and `_IORW` map to "none", "read", "write", and "readwrite"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_IO maps to "none", not _IOC. _IOC is a macro that puts all the components together to create a single IOCTL number.

//!
//! Again looking to the SPI `ioctl`s on Linux for an example, `SPI_IOC_NR_TRANSFER` is an `ioctl`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux doesn't specify SPI_IOC_NR_TRANSFER AFAIK. I guess this comes from rust-spidev ? Linux specifies SPI_IOC_MESSAGE, which is a macro that takes one argument :

#define SPI_IOC_MESSAGE(N) _IOW(SPI_IOC_MAGIC, 0, char[SPI_MSGSIZE(N)])

You can find this here : http://elixir.free-electrons.com/linux/latest/source/include/uapi/linux/spi/spidev.h#L116

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I knew that when I added this, but I needed some form of a name for this ioctl, so I just reused the same name that spidev used. Know of a good way to rephrase that that doesn't use SPI_IOC_NR_TRANSFER?

Copy link
Contributor

@roblabla roblabla Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't just doing

Again looking to the SPI `ioctl`s on linux for an example, [`SPI_IOC_MESSAGE(n)`]
(http://elixir.free-electrons.com/linux/v4.12.2/source/include/uapi/linux/spi/spidev.h#L116) is an
`ioctl` that queues up...

work ? (Also, maybe adding links to the kernel headers for various documentation bits would be nice, so readers can actually understand what symbols we're talking about).

//! ---------------------------
//!
//! For Linux, look at your system's headers. For example, `/usr/include/linux/input.h` has a lot
//! of lines defining macros which use `_IOR`, `_IOW`, `_IOC`, and `_IORW`. Some `ioctl`s are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and _IO. _IOC seems to be mostly used for arrays BTW.

@Susurrus
Copy link
Contributor Author

@roblabla Thanks for taking a look! I do have concerns that someone could pass in the wrong size in len for the ioctl!(* buf, ...) case. I was thinking like about an assert_ne!(len, len * ::std::mem::size_of::<$ty>()) to the buf ioctls, though I wonder if that would ban some valid use cases. This change I have in here is a breaking change that won't break at compile time, but subtly at runtime more likely.

@roblabla
Copy link
Contributor

roblabla commented Jul 17, 2017

I don't think it's a good idea to do that (what about if I want to pass an array of 1 u8 ?).

OK, maybe a stupid idea, but maybe the buf cases could actually take a slice ? I doubt there are many cases of buf where a NULL pointer would be sent (Please correct me if I'm wrong ? What would be the valid len of a null pointer ? 0 ?), and it'd avoid degrading back to C ergonomics.

At least for the spidev case, it would work, as the kernel verifies the length is 0 before trying anything on the array http://elixir.free-electrons.com/linux/latest/source/drivers/spi/spidev.c#L340

@Susurrus
Copy link
Contributor Author

@roblabla Yeah, I was thinking having it as a slice would be good. This is actually how rust-spidev uses these ioctls, it's all slices. But again, ioctls is such a long list, I'm not certain if NULL is necessary and I don't want to break stuff. In theory people who upgrade will have failures in some of their ioctl! macro calls and will need to go through and fix some, so it's unlikely this wouldn't be caught by that change.

I think it's reasonable to go ahead and make * buf versions take a buf slice, so I'll go and change that. Also, I should have addressed all of your other comments in the new version I just pushed. Thanks for those edits!

//!
//! ```
//! # #[macro_use] extern crate nix;
//! # use nix::libc::TIOCEXCL as TIOCEXCL;
//! ioctl!(bad none tiocexcl with TIOCEXCL);
//! # const SPI_IOC_MAGIC: nix::libc::c_int = 'k' as nix::libc::c_int;
Copy link
Contributor

@roblabla roblabla Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing that just popped up. Maybe we shouldn't hide this ? I fear that reading the docs, it's not immediately obvious that SPI_IOC_MAGIC is just another expression if we hide this.

Also, I think b'k' works better, rather than casting with as

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPI_IOC_MAGIC is actually defined in /usr/include/linux/spi/spidev.h, so I could expose it and just add a comment for it. SPI_IOC_NR_TRANSFER isn't defined, however, but I think I should expose that as well.

@Susurrus
Copy link
Contributor Author

@roblabla I tried to be more explicit about the various SPI constants used. Hopefully this is a lot more clear. Let me know what you think!

@roblabla
Copy link
Contributor

roblabla commented Jul 17, 2017

Looks great ! I find this much clearer than the older ones :)

:shipit: :D

@Susurrus
Copy link
Contributor Author

@oposborne This has changed a bit since you r+'d this, you want to have another look over it? If not, just let bors know.

@Susurrus
Copy link
Contributor Author

Hmmm, this looks wrong for ioctl!(write...) given that #626 changed it. Will need to look into this a bit more.

@Susurrus
Copy link
Contributor Author

I've been looking over #626 to understand why that change was made, and while it looks like the docs for ioctl all suggest that the third argument is a pointer (though they argue over what kind of pointer it is), it looks like some ioctls actually shove an integer value in there instead of a valid pointer. So that's cool.

I think the question now is how do we want to support that? I know that both forms are needed, so maybe we're just bikeshedding on names? Maybe split write into write_val and write_ptr or maybe write_int and write, since this is supposed to be a pointer, the default should stay for the pointer type.

cc @bkchr as you originally reported #626 and would like to get your feedback here.

There two different write semantics used with ioctls: one involves
passing a pointer the other involves passing an int. Previously the
ioctl! macro did not distinguish between these cases and left it up
to the user to set the proper datatype. This previous version was
not type safe and prone to errors. The solution here is to split the
"write" variant into a "write_ptr" and "write_int" variant that makes
the semantics more explicit and improves type safety by specifying
arguments better.
Instead of relying on the macro user to calculate the length in bytes
do that within the macro itself
@Susurrus
Copy link
Contributor Author

Just updated because of conflicts from some merges last night.

@roblabla @posborne Any last comments on these changes before I merge them?

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs are much improved. But I'm still left with a few questions:

  • How do you use ioctl! for ioctls that have more than 3 arguments?
  • How do you use ioctl! for read or write ioctls that take a pointer to struct argument? I think I understand, but it would be helpful to add such an example to the compile tests near the top of test_ioctl.rs

pub unsafe fn $name(fd: $crate::libc::c_int,
data: &mut [$ty])
-> $crate::Result<$crate::libc::c_int> {
convert_ioctl_res!($crate::libc::ioctl(fd, ior!($ioty, $nr, data.len() * ::std::mem::size_of::<$ty>()) as $crate::sys::ioctl::ioctl_num_type, data))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the size of data fixed when the macro is invoked, or can different length slices be passed into the generated function on different calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different length slices can be passed in on different calls. The ioctl for SPI_IOC_MESSAGE that I give as an example is used like that (you'd use it like ioctl(file, SPI_IOC_MESSAGE(2), xfer) where xfer is a spi_ioc_transfer[2]). You tell it how many messages and that's actually part of the calculated ioctl number. Would adding the C-equivalent help make this more clear or should I explain it better in the docs somehow?

/// The datatype used for the ioctl number
#[cfg(any(target_os = "android", target_env = "musl"))]
#[doc(hidden)]
pub type ioctl_num_type = ::libc::c_int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? Musl defines the request as an int, even when it's running on Linux? Baffling!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain if this is sarcastic or not, but yes, musl defines it as an int. Why are you surprised by that?

@Susurrus
Copy link
Contributor Author

Thanks for the review, @asomers! Regarding your specific questions:

How do you use ioctl! for ioctls that have more than 3 arguments?

I've never seen an example or docs that suggest this is a thing on POSIX systems, but I'm not an expert on ioctls, so I could definitely be wrong here. While I always see it defined like int ioctl(int fd, unsigned long request, ...);, with the third argument unspecified, the docs always say it's a pointer and never refer to any additional arguments.

That being said, I would consider it out-of-scope for this change to handle more than 1 request argument given that the previous code didn't either and this was supposed to be simply a documentation PR.

How do you use ioctl! for read or write ioctls that take a pointer to struct argument? I think I understand, but it would be helpful to add such an example to the compile tests near the top of test_ioctl.rs

You use ioctl!(read ..) and ioctl!(write_ptr ...) for this case. I provide an example working with reading using a struct pointer: ioctl!(bad read tcgets with TCGETS; termios);. Using write_ptr will look similar to the example I give in the "Working with arrays" section: ioctl!(write_buf spi_transfer with SPI_IOC_MAGIC, SPI_IOC_TYPE_MESSAGE; spi_ioc_transfer);, where instead of a primitive you provide the datatype name. I wouldn't think that would need very much explicit documentation because it follows exactly from how primitives are used.

If instead you're referring to how to use the generated function, there are two examples I have in here that show the generated function which both take pointers as their second arguments, so I don't provide a specific usage example of it.

@Susurrus
Copy link
Contributor Author

@asomers I just pushed a new commit that adds simple compile-tests of the generated functions for all the ioctls that it was easy to find examples for. So I skipped bad readwrite, read_buf, or readwrite_buf, as I couldn't find any clear examples of these. If anyone knows of any off the top of their head, please chime in and I'll add them.

Though I do think that bad readwrite can probably be removed as it's the same as bad write, I just added it for consistency. It doesn't offer any type safety over bad write either. The write versus readwrite distinction is only used for newer ioctls that use a different ioctl number generation for both.

@Susurrus
Copy link
Contributor Author

@asomers I'd love to get these tests working on FreeBSD as well since we claim to support more than just Linux/Android for ioctls. Do you have any suggestions for some ioctls that I could use?

@Susurrus
Copy link
Contributor Author

We're getting runtime failures because the ioctls numbers are being rejected by the kernel. I think these should work across all Linux platforms. The ioctls that our macros are generating, however, vary per target, which I'm not certain is correct.

So I decided to try something a little ambitious here; I'm going to try to use a build script to generate the ioctl numbers for each platform by compiling C functions that return various ioctl numbers. Then we can call into those functions from our Rust ioctl test code. This should get rid of those annoying hard-coded values and also make sure things are correct when we add new platforms. Since these calculations don't rely on any specific ioctls being available on the platform, just generating the numbers, they should be perfectly portable.

@asomers
Copy link
Member

asomers commented Jul 20, 2017

I can't find any examples of ioctls that take more than 3 arguments either, despite it technically being allowed. And you're right, the man page suggests that it shouldn't happen. So let's forget about supporting that weird use case. And the tcgets example looks good to me.

As for FreeBSD testing, most of the terminal control ioctls are somewhat portable, and most should work on ptys. Here's a partial list of ioctls that I found defined on both FreeBSD and Linux: TIOCEXCL TIOCNXCL TIOCSCTTY TIOCGPGRP TIOCSPGRP TIOCOUTQ TIOCSTI TIOCGWINSZ TIOCSWINSZ TIOCMGET TIOCCONS TIOCPKT TIOCNOTTY TIOCSETD TIOCGETD TIOCSBRK TIOCCBRK TIOCGSID TIOCGPTN TIOCSIG.

As for your test failures, I think it's untrue that all ioctls have the same value across Linux platforms. Some platforms, like mips, are unnecessarily divergent.

Shouldn't gcc be a dev-dependency instead of a build-dependency?

@Susurrus
Copy link
Contributor Author

As for FreeBSD testing, most of the terminal control ioctls are somewhat portable, and most should work on ptys. Here's a partial list of ioctls that I found defined on both FreeBSD and Linux: TIOCEXCL TIOCNXCL TIOCSCTTY TIOCGPGRP TIOCSPGRP TIOCOUTQ TIOCSTI TIOCGWINSZ TIOCSWINSZ TIOCMGET TIOCCONS TIOCPKT TIOCNOTTY TIOCSETD TIOCGETD TIOCSBRK TIOCCBRK TIOCGSID TIOCGPTN TIOCSIG.

Cool, I'll expose all of the TTY ioctls I'm using for testing on FreeBSD as well.

Shouldn't gcc be a dev-dependency instead of a build-dependency?

build-dependencies is for dependencies for the build-scripts themselves (so the build.rs build, not for compiling the actual library). dev-dependencies are not available in build.rs.

@asomers
Copy link
Member

asomers commented Jul 20, 2017

But build-dependencies are going to be pulled in every time nix is compiled by a dependent crate, right? It would be a shame to make all of our dependent crates depend on gcc, when it's only our tests that need to build a C extension.

@Susurrus
Copy link
Contributor Author

I think the solution is to integrate this into nix-test. I think that's why it actually exists, to work around this. If we go down this road, I'll integrate this testing into nix-test so it won't be a dependency for building.

@emberian
Copy link
Contributor

@Susurrus see also https://github.com/cmr/ioctl/tree/master/etc, the original source of this macro. I have scripts there for parsing the ioctl numbers out of the C headers. https://github.com/cmr/ioctl/blob/master/src/platform/linux-generated-x86_64.rs has all of them, but most of them are commented out for lacking the structs.

@Susurrus
Copy link
Contributor Author

@cmr Thanks, but I think this solution will work out. I'd prefer to not manually parse and instead run the C code directly. As an aside, why did you deprecate your ioctl crate? It would have been nice to have it around to provide these implementations for my serialport-rs crate and other crates would likely use it.

But even with these checks passing on all targets, it appears that the ioctl number isn't liked by the kernel. I wonder if those ioctl's really don't exist on those platforms. I'll have to dig into this further, but I think I'm going to reduce this PR down a bit and we can look into improving ioctl testing in another one.

@Susurrus
Copy link
Contributor Author

Okay, so I've removed the comparison of generated ioctl numbers and made ENOSYS be valid for the runtime calls for now. This should pass CI at this point and then we can merge!

@asomers The runtime ioctl calls are passing for FreeBSD, so that's cool!

@asomers
Copy link
Member

asomers commented Jul 21, 2017

Cool! I think we have a winner.

bors r+

bors bot added a commit that referenced this pull request Jul 21, 2017
670: Improve ioctl docs r=asomers

Integrates suggestions and comments made in #641. Basically we hid a lot of the internal workings of things and also revised the docs to make it more clear the exact API that `nix` is exposing.

There is a small amount of code cleanup I did in the macros. I also fixed the `bad` version of the `ioctl!` macro and also added a `bad none` version for use with no-data, hardcoded-number `ioctl`s.

Would appreciate any and all feedback. Please especially fetch this code locally and generate the pretty docs for it (`cargo doc --no-deps --open`) so you can see what our users will see.

Closes #641.
Closes #573.

cc @cmr @roblabla
@bors
Copy link
Contributor

bors bot commented Jul 21, 2017

@bors bors bot merged commit 4525567 into nix-rust:master Jul 21, 2017
@Susurrus Susurrus deleted the ioctl_docs branch July 21, 2017 02:34
@posborne
Copy link
Member

posborne commented Jul 21, 2017

Thanks for pushing this through @Susurrus and everyone else. I've been a bit busy with other things lately, but I am appreciative of the diligent work you all have been doing.

@Susurrus
Copy link
Contributor Author

Thanks, @posborne! And I appreciate you chiming in with your edits. Definitely helps!

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.

Improve documentation for ioctl Additional examples for ioctl
6 participants