libudev: Get all sysfs attrs for a device
cover libudev: Get all sysfs attrs for a device
Hi folks,
I recently took part in writing an application using libudev to retrieve information on devices present on a linux system. It became evident that apparently there is no way to retrieve all possible sysfs attributes for a particular device (the same way it is possible to get all of its properties). The matter kept nagging me -- even though we worked our way around this using sysfs directly -- and I decided to add a udev_device_get_sysattr_list_entry. For this matter the behavior of udev_device_get_sysattr_value had to be modified (a little): on first access of any sysattr, it create an list including _all_ sysfs-attributes as name (with an empty value, except for the symlinks). The values are cached as soon as thery're requested. Negative entries do not exist anymore.
Cheers, Thomas
On Thu, Mar 3, 2011 at 10:17, Thomas Egerer <[email protected]> wrote:
First, the patch seems line-wrapped.
Quoted 4 lines
I recently took part in writing an application using libudev to retrieve information on devices present on a linux system. It became evident that apparently there is no way to retrieve all possible sysfs attributes for a particular device (the same way it is possible to get all of its properties).
Keep in mind, that if you are root, you can do really nasty things if you open/read binary files. Sysfs is not meant to be used to blindly open "random" files. Think of it as device ioctl()s -- nobody would expect to just issue all numbers to a device, to check what comes back :) Things can go very wrong here.
It is actually the reason libudev does not support anything like this. People must exactly know what they want to read, and need to be very careful here, unlike it it with udev properties managed by udev.
If this is about a generic device browser, you would need to make sure nobody does weird things with it.
Quoted 2 lines
The matter kept nagging me -- even though we worked our way around this using sysfs directly -- and I decided to add a
udev_device_get_sysattr_list_entry.
I don't mind in general to have such a list. But we need to think about the above first, if that is really stuff we should offer.
Quoted 6 lines
For
this matter the behavior ofudev_device_get_sysattr_valuehad to be modified (a little): on first access of any sysattr, it create an list including _all_ sysfs-attributes as name (with an empty value, except for the symlinks). The values are cached as soon as thery're requested. Negative entries do not exist anymore.
The same code is used by udevd. For performance reasons, we can not afford to call readdir() for every device we look at. We don't want to cache anything which we don't need.
Thanks,
Kay
Kay Sievers [2011-03-03 15:11 +0100]:
Quoted 5 lines
Keep in mind, that if you are root, you can do really nasty things if you open/read binary files. Sysfs is not meant to be used to blindly open "random" files. Think of it as device ioctl()s -- nobody would expect to just issue all numbers to a device, to check what comes back :) Things can go very wrong here.
The current patches don't actually open the attribute files, just read the dir and stat the files; but I guess that was meant as a general "never try to iterate over all of them" warning.
Quoted 3 lines
The same code is used by udevd. For performance reasons, we can not afford to call
readdir()for every device we look at. We don't want to cache anything which we don't need.
Indeed, and I actually see no reason why udev_device_get_sysattr_value() would cause the building of the cache; shouldn't that be done in the new udev_device_get_sysattr_list_entry() instead, where it actually belongs?
(Note that you have the wrong function name in the doc comment of udev_device_get_sysattr_list_entry()).
Martin
Signature · 2 lines
Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Quoted 10 lines · Martin Pitt
Kay Sievers [2011-03-03 15:11 +0100]:
Quoted 5 lines
Keep in mind, that if you are root, you can do really nasty things if you open/read binary files. Sysfs is not meant to be used to blindly open "random" files. Think of it as device ioctl()s -- nobody would expect to just issue all numbers to a device, to check what comes back :) Things can go very wrong here.
The current patches don't actually open the attribute files, just read the dir and stat the files; but I guess that was meant as a general "never try to iterate over all of them" warning.
Sure, it's not about the patch, it's about what this patch offers to users, and might suggest the wrong thing to do.
Quoted 8 lines
Quoted 3 lines
The same code is used by udevd. For performance reasons, we can not afford to call
readdir()for every device we look at. We don't want to cache anything which we don't need.Indeed, and I actually see no reason why
udev_device_get_sysattr_value()would cause the building of the cache; shouldn't that be done in the newudev_device_get_sysattr_list_entry()instead, where it actually belongs?
Maybe. As said, we shoudl be careful here. Sysfs is really not meant to be blindly iterated over. But if there is a good use case, we can certainly do that. The "udevadm info -a" attribute stuff might even use that.
Kay
Kay,
The approach shown in the patch may have indeed a performance impact which we understand must be avoided. But I take it you are not generally opposed to providing such an interface, assuming it is implemented in a way that does not interfere with the existing mode of operation?
Let me explain a little bit why we think this interface would be helpful... We want to write unit tests for applications that interact with udev (get notified when devices pop up/disappear), and for this we need an abstraction layer that we can mock. libudev has a nice clean interface that fits this purpose very well actually, but we cannot obtain a list of attributes through libudev (why we sometimes need that, see below). This means that we are building a secondary abstraction to just obtain the list of attributes, and strictly funnel everything else through libudev. We then provide "mock" and "real" implementations of both, and make sure to "tie" the two different implementations properly (which deeply interact for the "mock" case under the hood). As you can imagine, this is "a bit" messy.
The test cases to be injected into the applications are usually generated from "traces" captured from live systems, possibly filtered and post-processed to simulate exactly the interaction we want to see. It is sometimes useful to "recapture" from test-data instead of a live system, and when we capture things, we don't always know what attributes a subsequent test might actually want. To faithfully replay the interaction later we therefore need to preserve "as much as we can".
We are aware that poking random things in sysfs can have rather "undesirable" consequences, so there is probably not much one can do with such an interface outside the scope of such testing scenarios. It is on the other hand very difficult to provide good testability without it.
Cheers, Thomas
Quoted 4 lines · Thomas Egerer
The approach shown in the patch may have indeed a performance impact which we understand must be avoided. But I take it you are not generally opposed to providing such an interface, assuming it is implemented in a way that does not interfere with the existing mode of operation?
Sure, sounds fine to have that.
Maybe just use the interface for:
udevadm info -a
and replace:
http://git.kernel.org/?p=linux/hotplug/udev.git;a=blob;f=udev/udevadm-info.c;hE10f4aa907dc56a22544564ec1e10045046bf61;hb=HEAD#l34
with the same patch, and check that all works fine.
Quoted 20 lines
Let me explain a little bit why we think this interface would be helpful... We want to write unit tests for applications that interact with udev (get notified when devices pop up/disappear), and for this we need an abstraction layer that we can mock. libudev has a nice clean interface that fits this purpose very well actually, but we cannot obtain a list of attributes through libudev (why we sometimes need that, see below). This means that we are building a secondary abstraction to just obtain the list of attributes, and strictly funnel everything else through libudev. We then provide "mock" and "real" implementations of both, and make sure to "tie" the two different implementations properly (which deeply interact for the "mock" case under the hood). As you can imagine, this is "a bit" messy.
The test cases to be injected into the applications are usually generated from "traces" captured from live systems, possibly filtered and post-processed to simulate exactly the interaction we want to see. It is sometimes useful to "recapture" from test-data instead of a live system, and when we capture things, we don't always know what attributes a subsequent test might actually want. To faithfully replay the interaction later we therefore need to preserve "as much as we can".
Quoted 4 lines
We are aware that poking random things in sysfs can have rather "undesirable" consequences, so there is probably not much one can do with such an interface outside the scope of such testing scenarios. It is on the other hand very difficult to provide good testability without it.
Sounds all fine, just be careful. :)
Kay
Kay Sievers schrobtete:
Quoted 6 lines · Thomas Egerer
Quoted 4 lines
The approach shown in the patch may have indeed a performance impact which we understand must be avoided. But I take it you are not generally opposed to providing such an interface, assuming it is implemented in a way that does not interfere with the existing mode of operation?
Quoted 6 lines
Sure, sounds fine to have that.
Maybe just use the interface for:
udevadm info -a
and replace:
http://git.kernel.org/?p=linux/hotplug/udev.git;a=blob;f=udev/udevadm-info.c;hE10f4aa907dc56a22544564ec1e10045046bf61;hb=HEAD#l34
In regard to your objection concerning performance, I rewrote the original patch, using a separate list for the means of retrieving all sysfs attributes. The newly introduced interface is now being used in udevadm, too. Patches (not linewrapped, this time) follow.
Cheers, Thomas
Quoted 4 lines · Thomas Egerer
In regard to your objection concerning performance, I rewrote the original patch, using a separate list for the means of retrieving all sysfs attributes. The newly introduced interface is now being used in udevadm, too. Patches (not linewrapped, this time) follow.
Applied, with a few variable renames, a missing closedir(). We don't magically return the type of the attribute: reading a non-core link will fail just fine. The newly added entries to the blacklist in the info command makes sure we don't return the core links.
Thanks,
Kay