Refactor the ioctl API and documentation #833

Susurrus · 2018-01-08T02:38:31Z

I still need to flesh out the docs for the different ioctl_*! variants that now exist. I separated them into different macros so they can have their own docs, which should help discoverability. And when macro namespacing comes around this will be a pretty neatly documented API I think (though we'll likely want to rename these macros again at that point).

Split ioctl! into separate macros. This makes documentation easier to read.
Expose request_code_*! in the documentation to make the ioctl_*_bad macros easier to use.
Reorganize the file hierarchy to be simpler

cc @gabrielesvelto @posborne @jethrogb

Susurrus · 2018-01-19T05:01:55Z

I think I've gotten this into its final form. By clarifying in every ioctl wrapper generator macro what the macro arguments are, what the generated function will look like, and an example if I've found one, we end up with I think some very solid docs!

Right now I'm missing examples (and tests) for ioctl_readwrite_buf!, ioctl_read_buf!, and ioctl_readwrite_bad!, so I'd also appreciate if anyone knows of any ioctls like those that I could add.

I'd encourage anyone viewing this to pull this branch and cargo doc --open, as I'd very much appreciate some double-checks on the final formatted output.

posborne · 2018-01-21T03:55:07Z

Read through everything and I think this is a significant improvement on the documentation front (including very helpful examples). I also like the design of the macros versus the overloaded macros we have previously with the unconventional "with" syntax.

I think the biggest decision here is whether it makes sense to just drop the old macro or if we should provide a mapping from the old macros to the new ones and mark the old ioctl! macro as deprecated.

Susurrus · 2018-01-21T05:56:32Z

Great! I thought this makes things a lot better too, so I'm glad someone else read through and confirmed that!

Regarding maintaining the old macro through a deprecation period, I would be onboard except that I'd need to whip up a ton of new documentation for the ioctl! macro to make sure it was properly documented, or would you say punt on the documentation and just say, use these other functions instead?

jethrogb · 2018-01-21T18:01:01Z

src/sys/ioctl/mod.rs

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


_IORW is actually _IOWR

jethrogb · 2018-01-21T18:01:45Z

src/sys/ioctl/mod.rs

@@ -107,14 +107,12 @@
 //! [`ioctl_list` man page](http://man7.org/linux/man-pages/man2/ioctl_list.2.html) describes a


_IORW is actually _IOWR

jethrogb · 2018-01-21T18:05:47Z

src/sys/ioctl/mod.rs

-//! There is also a `bad none`, `bad write_int`/`bad write_ptr`, and `bad readwrite` variant that work
-//! similar to the standard `none`, `write_int`/`write_ptr`, and `readwrite` variants.
+//! The `ioctl_*_bad` variants should also be used when `ioctl` numbers are generated using the
+//! wrong macro. For example, the `KVM_CREATE_VM` ioctl is defined as follows in C:


In this example, the ioctl is not using the wrong macro. The R/W in _IOR, _IOW, _IOWR means that in addition to the parameter passed to ioctl, user memory is to be read/written. Since this ioctl doesn't pass a pointer, no additional user memory is involved, so there is nothing to be read or written.

So the mapping should look like:
no argument (implicit 0) -> _IO
ulong argument -> _IO
pointer argument -> _IOR/_IOW/_IOWR

I believe that's what you're saying?

Yes. This seems to also be the case for (some of the) BSDs as @myfreeweb pointed out.

valpackett · 2018-01-28T19:56:56Z

From #825: writing integers should be of type VOID on FreeBSD and DragonFly:

https://github.com/freebsd/freebsd/blob/7982612fd5e8ef311a2a58178893f54e6a0601a4/sys/sys/ioccom.h#L59

https://gitweb.dragonflybsd.org/dragonfly.git/blob/c3c55883c35bc4a6ebdf01eab8725c83884536c9:/sys/sys/ioccom.h#l57

example of usage:

https://github.com/freebsd/freebsd/blob/d8596f6f687a64b994b065f3058155405dfc39db/sys/sys/consio.h#L357-L363

Susurrus · 2018-01-29T02:40:36Z

@myfreeweb or @asomers Could either of you provide correct values for _IOWINT('v', 4) and _IOWINT('p', 2) on both x64 and i686 FreeBSD? I want to write some tests to make sure our calculations are correct on both.

Susurrus · 2018-01-29T03:10:40Z

@asomers and @myfreeweb I've added a commit that should rectify the ioctl_write_int problem, but the tests will fail until I get the proper values from either of you to test against.

I still need to fix #824 here and also jethrog's comment above.

valpackett · 2018-01-29T10:35:36Z

0x20047604 and 0x20047002 on amd64 and arm64.

I don't have an actual 32 bit machine right now, but just cross compiling with the -m32 flag produces the same values. (I haven't seen any 32/64 bit conditionals in the ioctl headers either…)

Susurrus · 2018-01-30T16:59:14Z

@myfreeweb Thanks for those constants, looks like they're correct for both x86 and x86_64 FreeBSD platforms (we don't test on DragonFlyBSD).

* Split `ioctl!` into separate macros. This makes documentation easier to read. * For every `ioctl_*!` macro include a description of the macro arguments as, the function prototype for the generated wrapper function, and an example if we have one. * Expose `request_code_*!` in the documentation to make the `ioctl_*_bad` macros easier to use. * Reorganize the file hierarchy to be simpler

ioctls on FreeBSD and DragonflyBSD have a separate request code generation macro `_IOWINT` which is now exposed as `request_code_write_int`. `ioctl_write_int` is also fixed on these platforms to use this new request

While usually `ioctl()` passes a pointer, the function call has been overloaded to allow integers to be passed. For some platforms this is an `int` and on others it's a `ulong`. Fixes nix-rust#824.

Susurrus · 2018-04-10T15:37:12Z

Okay, did another pass through this PR and I think I've fixed all outstanding issues. Since the last time I added a CHANGELOG entry for the last commit and removed the section about "bad" ioctl's applying to ioctl's that used the wrong macros as I don't have a concrete example.

posborne · 2018-04-11T05:29:07Z

@Susurrus Looks great to me.

bors r+

@gabrielesvelto

833: Refactor the ioctl API and documentation r=posborne a=Susurrus I still need to flesh out the docs for the different `ioctl_*!` variants that now exist. I separated them into different macros so they can have their own docs, which should help discoverability. And when macro namespacing comes around this will be a pretty neatly documented API I think (though we'll likely want to rename these macros again at that point). * Split `ioctl!` into separate macros. This makes documentation easier to read. * Expose `request_code_*!` in the documentation to make the `ioctl_*_bad` macros easier to use. * Reorganize the file hierarchy to be simpler cc @gabrielesvelto @posborne @jethrogb Co-authored-by: Bryant Mairs <bryantmairs@google.com>

Susurrus · 2018-04-11T05:34:05Z

Just realized this is missing a CHANGELOG entry to identify that the ioctl! macro has changed to the ioctl_* macros. But I can do that as a separate commit.

bors · 2018-04-11T05:57:44Z

Build succeeded

882: Update CHANGELOG for #833 r=Susurrus a=Susurrus Forgot to mention the big changes in #833 before it was merged. Co-authored-by: Bryant Mairs <bryantmairs@google.com>

Susurrus force-pushed the ioctl_refactor branch from cb268f0 to 8ea60bc Compare January 11, 2018 02:31

Susurrus force-pushed the ioctl_refactor branch from 8ea60bc to 978790f Compare January 19, 2018 04:58

Susurrus changed the title ~~WIP: Refactor the ioctl API and documentation~~ Refactor the ioctl API and documentation Jan 19, 2018

Susurrus force-pushed the ioctl_refactor branch 5 times, most recently from 6b0d824 to 7eb8dd1 Compare January 19, 2018 06:33

jethrogb reviewed Jan 21, 2018

View reviewed changes

Susurrus mentioned this pull request Jan 28, 2018

FreeBSD: cfmakesane, EVFILT_* #825

Merged

Susurrus force-pushed the ioctl_refactor branch from 7eb8dd1 to 02e8367 Compare January 29, 2018 02:18

Susurrus force-pushed the ioctl_refactor branch from 02e8367 to cff0182 Compare January 29, 2018 03:04

Susurrus force-pushed the ioctl_refactor branch from cff0182 to 957ec87 Compare January 30, 2018 16:38

Susurrus mentioned this pull request Jan 30, 2018

ioctl write_int doesn't support passing long integers #824

Closed

Susurrus force-pushed the ioctl_refactor branch from 957ec87 to f91530e Compare January 31, 2018 03:56

Bryant Mairs added 3 commits April 10, 2018 08:28

Implement equivalent for _IOWINT for FreeBSD-like targets

1a815f1

ioctls on FreeBSD and DragonflyBSD have a separate request code generation macro `_IOWINT` which is now exposed as `request_code_write_int`. `ioctl_write_int` is also fixed on these platforms to use this new request

Correct the third argument to ioctl on appropriate platforms.

5dad660

While usually `ioctl()` passes a pointer, the function call has been overloaded to allow integers to be passed. For some platforms this is an `int` and on others it's a `ulong`. Fixes nix-rust#824.

Susurrus force-pushed the ioctl_refactor branch from f91530e to 5dad660 Compare April 10, 2018 15:36

bors bot merged commit 5dad660 into nix-rust:master Apr 11, 2018

Susurrus deleted the ioctl_refactor branch April 11, 2018 15:00

Susurrus pushed a commit to Susurrus/nix that referenced this pull request Apr 11, 2018

[skip ci] Update CHANGELOG for nix-rust#833

17123ed

Susurrus mentioned this pull request Apr 11, 2018

Update CHANGELOG for #833 #882

Merged

bors bot added a commit that referenced this pull request Apr 11, 2018

Merge #882

97e0c47

882: Update CHANGELOG for #833 r=Susurrus a=Susurrus Forgot to mention the big changes in #833 before it was merged. Co-authored-by: Bryant Mairs <bryantmairs@google.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the ioctl API and documentation #833

Refactor the ioctl API and documentation #833

Susurrus commented Jan 8, 2018

Susurrus commented Jan 19, 2018

posborne commented Jan 21, 2018

Susurrus commented Jan 21, 2018

jethrogb Jan 21, 2018

jethrogb Jan 21, 2018

jethrogb Jan 21, 2018

Susurrus Jan 30, 2018

jethrogb Jan 30, 2018

valpackett commented Jan 28, 2018

Susurrus commented Jan 29, 2018

Susurrus commented Jan 29, 2018

valpackett commented Jan 29, 2018

Susurrus commented Jan 30, 2018

Susurrus commented Apr 10, 2018

posborne commented Apr 11, 2018

Susurrus commented Apr 11, 2018

bors bot commented Apr 11, 2018

		@@ -107,14 +107,12 @@
		//! [`ioctl_list` man page](http://man7.org/linux/man-pages/man2/ioctl_list.2.html) describes a

Refactor the ioctl API and documentation #833

Refactor the ioctl API and documentation #833

Conversation

Susurrus commented Jan 8, 2018

Susurrus commented Jan 19, 2018

posborne commented Jan 21, 2018

Susurrus commented Jan 21, 2018

jethrogb Jan 21, 2018

Choose a reason for hiding this comment

jethrogb Jan 21, 2018

Choose a reason for hiding this comment

jethrogb Jan 21, 2018

Choose a reason for hiding this comment

Susurrus Jan 30, 2018

Choose a reason for hiding this comment

jethrogb Jan 30, 2018

Choose a reason for hiding this comment

valpackett commented Jan 28, 2018

Susurrus commented Jan 29, 2018

Susurrus commented Jan 29, 2018

valpackett commented Jan 29, 2018

Susurrus commented Jan 30, 2018

Susurrus commented Apr 10, 2018

posborne commented Apr 11, 2018

Susurrus commented Apr 11, 2018

bors bot commented Apr 11, 2018

Build succeeded