[PATCH 0/2] libudev: Get all sysfs attrs for a device

[PATCH 0/2] libudev: Get all sysfs attrs for a device

[Thomas Egerer]

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

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Kay Sievers]

On Thu, Mar 3, 2011 at 10:17, Thomas Egerer <thomas.egerer@secunet.com> wrote:

First, the patch seems line-wrapped.

> 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.

> 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.

> 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.

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

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Martin Pitt]

Kay Sievers [2011-03-03 15:11 +0100]:
> 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.

> 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

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Kay Sievers]

On Thu, Mar 3, 2011 at 18:00, Martin Pitt <martin.pitt@ubuntu.com> wrote:
> Kay Sievers [2011-03-03 15:11 +0100]:
>> 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.

>> 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?

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

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Thomas Egerer]

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

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Kay Sievers]

On Thu, Mar 3, 2011 at 18:38, Thomas Egerer <thomas.egerer@secunet.com> wrote:
> 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.

> 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.

Sounds all fine, just be careful. :)

Kay

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Thomas Egerer]

Kay Sievers schrobtete:
> On Thu, Mar 3, 2011 at 18:38, Thomas Egerer <thomas.egerer@secunet.com> wrote:
>> 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
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

Re: [PATCH 0/2] libudev: Get all sysfs attrs for a device

[Kay Sievers]

On Fri, Mar 4, 2011 at 17:06, Thomas Egerer <thomas.egerer@secunet.com> wrote:
> 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