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

Hotplug events on device connect and disconnect #5

Open
kevinmehall opened this issue Oct 29, 2023 · 16 comments · May be fixed by #20
Open

Hotplug events on device connect and disconnect #5

kevinmehall opened this issue Oct 29, 2023 · 16 comments · May be fixed by #20
Labels
enhancement New feature or request

Comments

@kevinmehall
Copy link
Owner

@kevinmehall kevinmehall added the enhancement New feature or request label Oct 29, 2023
@martinling
Copy link

I looked into working on this as it is one of the things we'd want to have in order to switch from rusb.

Avoiding a libudev dependency would be preferable for us, since we'd like the option to ship a static binary. The systemd developers insist that statically linking libudev is not allowed, because they want to be free to mess with the interface between the daemon and that library, so distros do not ship a static libudev.a.

For the direct kernel route, there is a kobject_uevent crate to help with unpacking the netlink messages.

However, I suspect there may be timing issues. If you listen to the netlink socket, you get the notifications of new devices at the same time the udev daemon does, but that may be before it has had time to apply rules, set permissions etc so the device may not be ready to open and use. I haven't verified that yet though.

@kevinmehall
Copy link
Owner Author

Well, there's the ugly option of spawning a udevadm monitor process and parsing its output, but that format could theoretically change too.

@kevinmehall
Copy link
Owner Author

Also came across https://github.com/cr8t/udev -- @cr8t started a pure-rust implementation of libudev just a few days ago, if we want to rely on the stability of udevd's socket messages.

@martinling
Copy link

As I understand it, the upstream position is that the udevd messages are subject to change, and that the only interface they promise to keep stable is the libudev API/ABI. So a rust reimplementation of libudev would be subject to the same risk as statically linking libudev: it may randomly break on a new systemd release.

@cr8t
Copy link

cr8t commented Nov 8, 2023

Also came across https://github.com/cr8t/udev -- @cr8t started a pure-rust implementation of libudev just a few days ago, if we want to rely on the stability of udevd's socket messages.

Still very early days, but my intentions are to upstream changes to nusb once the API is stable. I've made best effort to follow the implementation in eudev for a cross-platform, init-independent interface.

So a rust reimplementation of libudev would be subject to the same risk as statically linking libudev: it may randomly break on a new systemd release.

I've made an effort to follow eudev, so there are no systemd parts at all. The API/ABI is going to be different, because I have internal implementation differences, notably I use std::collections::LinkedList instead of constructing the list from raw pointers.

Currently, working on the public API helper functions. If there is something that rusb needs, and you don't see it, please let me know.

@kevinmehall
Copy link
Owner Author

Is it intended to work on distributions that use systemd-udevd, or only on distros with eudev?

@cr8t
Copy link

cr8t commented Nov 8, 2023

Is it intended to work on distributions that use systemd-udevd, or only on distros with eudev?

I don't know yet. Ideally, it will support as many platforms as possible. That may end up using something like a systemd compatibility layer hidden behind a feature flag, maybe not. Still too early to tell, and I'm not familiar enough with how systemd-udevd works to say what a roadmap would look like for sure.

@cr8t
Copy link

cr8t commented Dec 6, 2023

@kevinmehall udevrs v0.1.0 is published (had to rename because of naming conflicts). I can start work on a PR integrating with nusb.

@martinling
Copy link

It seems that the current UDEV_MONITOR_MAGIC value of 0xfeedcafe has not changed in quite some years, although it has changed in the past (I found a bug report caused by it changing in 2010). The value 0xcafe1dea has been used in the past, so could still be present on old systems. I'm not sure exactly when it changed or if there are other past values too.

Perhaps the socket interface between udev daemon implementations and libudev or similar clients could be considered de facto stable at this point. But it's dangerous to assume that; anything monitoring these messages should check the magic value and handle a mismatch - at least safely, ideally gracefully. @cr8t it doesn't look like your code currently checks the magic field - do you have any plan in mind for how to handle that?

@cr8t
Copy link

cr8t commented Dec 8, 2023

But it's dangerous to assume that; anything monitoring these messages should check the magic value and handle a mismatch - at least safely, ideally gracefully. @cr8t it doesn't look like your code currently checks the magic field - do you have any plan in mind for how to handle that?

I was not aware this magic value had changed at any point in the past, and missed handling a mismatch.

There is now a fix merged, and will be included in the next patch release.

There may still need to be some testing around the difference between htonl in C, and how I parse integers in the Rust implementation.

If there are users that have systems with different magic bytes, they are welcome to open an issue, and we can address it. Until then, I'm not that concerned about it.

@kevinmehall
Copy link
Owner Author

I can start work on a PR integrating with nusb.

That would be excellent!

It seems like if there's a magic number to detect protocol versions and the protocol hasn't changed in years (and across a fork), they're at least thinking about protocol compatibility even if they don't want to guarantee it. So while not using libudev as intented makes me a little uncomfortable, there are also major benefits of not having a C dependency.

@cr8t
Copy link

cr8t commented Dec 15, 2023

So while not using libudev as intented makes me a little uncomfortable, there are also major benefits of not having a C dependency.

For what it's worth, I've tried to stay as close as possible with libudev, and aiming for functional compatibility.

That would be excellent!

What code area should the hotplug logic go under? I'm guessing it will use the UdevMonitor functionality, which would require something that lives for the entire runtime of the program to hold ownership of a UdevMonitor struct. If the hotplug functionality can be dynamically enabled/disabled, ownership is needed for at least the entire time hotplug functionality is enabled.

@kevinmehall
Copy link
Owner Author

kevinmehall commented Dec 16, 2023

What code area should the hotplug logic go under?

nusb intentionally doesn't have a context object like libusb and rusb. Its function for listing devices is just a free function at the top level of the crate, and I imagine this being something similar.

Would an API like this make sense?

fn watch_devices() -> HotplugWatch;

struct HotplugWatch(platform::HotplugWatch); // Linux version owns UdevMonitor.

impl Iterator for HotplugWatch {
    type Item = HotplugEvent;
}

impl Stream for HotplugWatch {
    type Item = HotplugEvent;
}

/// Platform-specific opaque device identifier that can be used to see if two devices are the same,
/// or remove a disconnected device from a user-maintained set of `DeviceInfo`s. We'd add a new
/// `id` method to `DeviceInfo` to get its `DeviceId`. I assume we can't get all the `DeviceInfo` fields
/// for a disconnected device on all platforms, so a disconnect event is only the ID.
/// On Linux, this would probably be the `busnum` and `devnum`
#[derive(Clone, Eq, PartialEq, Hash)]
struct DeviceId(...);

enum HotplugEvent {
    Connected(DeviceInfo),
    Disconnected(DeviceId),
    Error(crate::Error),
}

Like the implementation of list_devices, the public function would just call into platform-specific code that could live under src/platform/linux_usbfs/udev.rs or something like that.

You could skip the async Stream and start with the blocking iterator if that makes things simpler. nusb comes with its own epoll reactor that could be adapted to watch the udev file descriptor, but right now it's somewhat specific to usbfs file descriptors.

Libusb hotplug allows filtering by a few fields like vendor ID and product ID. If there were APIs for that I assume UdevMonitor could apply those filters at the udev level. But I'm not sure it's necessary, because there aren't going to be enough USB events for performance to be an issue, and using.filter() on Iterator or Stream just checking DeviceInfo fields is probably more intuitive and flexible.

@kevinmehall kevinmehall linked a pull request Dec 27, 2023 that will close this issue
3 tasks
@kevinmehall
Copy link
Owner Author

Started implementing hotplug for Windows and macOS on #20.

@kevinmehall
Copy link
Owner Author

I've decided against using udevrs here. Firstly, its UdevMonitor doesn't work, though I think it could easily be fixed. But mainly, on taking a closer look at the source, it's a pretty literal translation of all of eudev/libudev from C to Rust. This is a problem both because it's very non-idiomatic Rust, and because eudev is LGPL.

Instead, I've gone ahead and added netlink socket support in rustix (which became its own yak shave), and wrote my own code to receive and parse the netlink messages from udev. It doesn't do the fancy in-kernel event filtering with BPF that libudev does, but it's the simplest thing that works, and there's no unsafe code outside of rustix.

@cr8t
Copy link

cr8t commented Mar 22, 2024

Firstly, its cr8t/udev#15, though I think it could easily be fixed.

I can dedicate some time to fixing that. It's the part of the code base I'm least confident about.

This is a problem both because it's very non-idiomatic Rust, and because eudev is LGPL.

Fair about the LGPL licensing. Regarding non-idiomatic Rust, I wanted to start with as close to a literal translation as possible, and then iterate on making the implementation idiomatic Rust. This is typically how C to Rust ports proceed.

Instead, I've gone ahead and added netlink socket support in rustix

That's awesome! rustix is an awesome crate, and I'll definitely integrate your netlink impl from there.

wrote my own code to receive and parse the netlink messages from udev. It doesn't do the fancy in-kernel event filtering with BPF that libudev does

I'll review your code, and see if I can do something similar in userland without BPF. Honestly, I would also prefer to avoid BPF if possible. Again, this is a holdover from the translation from eudev.

cr8t added a commit to cr8t/udev that referenced this issue May 10, 2024
Changes the license to LGPL-v2 or later after a re-reading of the `eudev`
license.

After discussion in
kevinmehall/nusb#5 (comment),
and reviewing the terms of LGPL, it seems required to also license
`udevrs` under LGPL.

`udevrs` is a very close translation of `eudev`. Regardless of how much
the project changes to be more idiomatic Rust, it will always have its
roots as a port of `eudev`.

IANAL, so to keep things simple let's just use the same license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants