pilebones / go-udev

Simple udev implementation in Golang
GNU General Public License v3.0
95 stars 28 forks source link

Support udev-based events #9

Closed stolowski closed 6 years ago

stolowski commented 6 years ago

Hi! I've managed to demistify the magic 'groups' constant - thanks to udevmonitor (part of systemd/udev) source code... It turns out the value of "1" that you had gives you kernel events, while "2" gives you back events that are processed by udev - much richer, with more attributes (such as vendor info, serial numbers and more). Generally this is the data you can see if you run udevadm monitor --udev -p. Some of that data you can later fish from /sys, however sysfs is pretty complicated and has many gotchas. Also, if you use udev events instead of kernel events, you get same rich data when the device is removed - something you won't get from sysfs anymore after the device gets unplugged.

Here is a draft of changes that make it usable from go-udev with request for comments. It works in monitor mode but there is still some work/polish to do: it would be nice to handle the binary header instead of skipping it (first 40 bytes). The crawler mode could also use udev (I think). Also, udevmonitor (the one from systemd/udev) does some credential checking which I don't understand yet.

Please let me know what you think and I'll iteratre on this a bit (but new crawler mode would likely be a separate PR).

pilebones commented 6 years ago

Hi, good news and explanations about groups constant, thanks a lot !

Note about code for a merge:

How do you estimate difficulty to implement a decoder of the head of kernel events ? I think it will be nice to have this feature to fully manage netlink events. If it is too long alone, we can work together on this feature if you wish.

Concerning a way to get the existing devices without checking and parsing files from sys-fs (as implemented), that's what I wanted to do initially using also the udev socket. But for lack of time and solutions, I aborted this feature. If you want to intiate this feature, no problem. Feel free but in another PR.

General question, do you still use this library for your open-source project (snapcore/snapd) ?

stolowski commented 6 years ago

Hi! Sure, I'll address your suggestions re comments.

Regarding the header (just for clarification, it's not a header defined by the kernel, it's a private header of udevd/libudev) it's problematic. It's not difficult to decode it, the problem is that it appears to be a private format of data exchange between udevd and its client tools (udevadm, via libudev), so it can change without notice (not a problem for udev, since its deamon and tools are from same source package), breaking 3rd party code; even the length of 40 that I temporarily hardcoded can change (the only way to know where actual data starts is to decode the header, it has data offset value). For the above reasons I'm not quite sure how to proceed with that.

Regarding existing devices, udevadm (via libudev) just iterates over /sys (like you do), but in addition it appends extra attributes from its own internal database (the same extra attributes you'd receive via netlink socket with my patch if the device was plugged). So it's difficult to have this functionality without libudev.

As for my project - yes, I'm trying to incorporate go-udev for our upcoming hotplug support, go-udev is currently forked in our source tree for convienience and I'm addressing all problems I find by proposing patches to you first before applying them locally. I'm not sure though if we will keep using go-udev or will try some different approach as the above (especially enumerating existing devices) is a serious issue (not your fault by any means), so we may end up with a solution that doesn't make sense for your general utility library; at the same time we don't want to use cgo-based packages; we will see.

Cheers!

stolowski commented 6 years ago

Allright, after some more investigation it turned out the udev data header is actually rather stable, for example it didn't change between Ubuntu 14.04 and 18.04 (that is, in four years); I haven't checked older versions.

I've updated the branch and consider it complete when it comes to monitoring. The parser now looks into specific fields of udev header. Added unit tests; they are a bit heavy when it comes to test data, but I think it makes sense to have at least a few checks based on real (raw) data with real headers.

When it comes to enumerating existing devices, in our project we will just parse 'udevadm info -e' output for the lack of good alternatives. Therefore I'll not propose any PR for this for go-udev/crawler as it's a bit specific use case, I don't think it fits your generic-purpose library. There is no good way of doing this other than proper integration with libudev (which means cgo).

pilebones commented 6 years ago

It is a good news for us if the header format not changed since 4 years, thank you for checking and for this parser.

I'm not against this kind of unit-test, it is not too heavy for me. A tested feature is better than no test. It is a good initiative.

Indeed, without using cgo to bind to existing udev libraries / binaries, it is not really possible to manage some feature or to access to the udev internal db. The /sys provide the only way I found to retrieve data (kernel struct as file) related to udev. I was aware about this limitation when I started the project but I wished re-implemnted as much as possible udev using Go to keep control and to keep control of our implementation.