Use pass by value for write ioctls

[bkchr]

The kernel expects that the values are passed by value and not by pointer for writing ioctls.

[asomers]

Can you give an example of a an ioctl that was improperly defined with the old macro?

[bkchr]

Using the following ioctl:

const HCI_IOC_MAGIC: u8 = 'H' as u8;
const HCI_IOC_HCIDEVUP: u8 = 201;
ioctl!(write hci_dev_up with HCI_IOC_MAGIC, HCI_IOC_HCIDEVUP; std::os::raw::c_int);

It is defined here: http://elixir.free-electrons.com/linux/latest/source/include/net/bluetooth/hci_sock.h#L68

By applying my patch, the ioctl finally worked, otherwise I got a enodev error. By comparing strace of a c program doing the same operation and my rust program, I saw that the difference was the last argument to ioctl. For the c strace it was just "0" and for the rust implementation it was a pointer.

Are there any ioctl that expect a pointer?

[Susurrus]

So it definitely seems that lack of test coverage is part of this problem. I'd like to see some tests included here that can confirm this works as expected. Is that possible to add some that will work in CI?

[bkchr]

Hmm, I think that you will need root access and CI will probably not give you root. ^^

[asomers]

Not necessarily. Plenty of ioctls don't require root access, for example terminal control ioctls. All we need here is test coverage for one ioctl of each type supported by the ioctl! macro.

[Susurrus]

@bkchr Just wanted to ping you on this as we'd love to get this merged.

[bkchr]

Yeah I also would like to get this merged. I already searched for a list of ioctls that don't need root access, but the search was not successful. Does someone of you have a list of ioctls that I could use? I just need them and would add the test cases.

[asomers]

@bkchr there's no master list of IOCTLs. Anybody can define a new one. But if you do "grep -R SIOC /usr/include" you should find plenty.

[Susurrus]

I use the ioctl-rs in my projects as it implements ioctl's directly. I'd encourage you to sample from their list. My projects specifically use tiocnxcl, tiocexcl, tiocmbis, tiocmbic, and tiocmget. None of those should require root access, though I think you'll have to use them on a serial device, so you'd need to use openpty to generate one and then call those on them.

With the ioctl crate (different than the one I linked to) suggesting people to this crate, having some substantial tests/examples of ioctls will help people migrate.

[posborne]

Good catch @bkchr. I'm OK with getting this merged without doing the additional work on tests off the bat since this does appear to be a problem. We might need to look at users of nix to see what the impact might be, but this could be a breaking change in some cases.

I think the only other requirement before merging would be to add a note on the changing (and its breaking potential) to the CHANGELOG.md file.

Looking at some of my own code using this, I see at least one case where I will need to update the calling code (spidev_transfer_buf) to work with this change and a few cases where the calls are likely broken without this change (set_max_speed_hz, ...): https://github.com/rust-embedded/rust-spidev/blob/master/src/spidevioctl.rs#L145

[Susurrus]

Yes, since this appears to be a bug, let's get this added to the list of Fixed changes.

[bkchr]

Okay I changed the Changelog :) I think that if you need a pointer for a write, you can use write buf.
Finding some ioctls that use write pass by value is not that easy :/ So I don't added tests.

[posborne]

bors r+

[bors]

Timed out

[posborne]

bors r+

[posborne]

bors r-

[bors]

Canceled

[posborne]

bors r+

[bors]

Build succeeded

• buildbot/nix-rust/nix fbsd11
• continuous-integration/travis-ci/push