search

dcuddeback / libudev-rs

Rust wrapper for libudev
MIT License
23 stars 16 forks source link

Take a look at pyudev issues that have been fixed #8

Closed mulkieran closed 7 years ago

mulkieran commented 7 years ago

Your implementation makes some mistakes that pyudev made too, that were eventually discovered by means of extensive testing.

Two that I think I see in this implementation that pyudev used to make are:

I'm happy to refer you to more specific information if you are interested.

https://github.com/pyudev/pyudev

emberian commented 7 years ago

@dcuddeback are you still maintaining this library? do you intend to fix these? if not to either, mind if I do it?

mulkieran commented 7 years ago

@cmr Hi! I'ld be happy to try to assist with this. In the case of pyudev I wasn't actually the original developer, lunaryorn eventually gave me full ownership as he had moved on to other projects. Perhaps @dcuddeback would be willing to do something similar?

dcuddeback commented 7 years ago

@mulkieran Could you explain what you mean about it not being "meaningful" to iterate through a device's attributes?

mulkieran commented 7 years ago

@dcuddeback Hi, here are a couple of references:

Description of problem:

udev_device_get_sysattr_list_entry returns a list of attribute, value pairs. The values are NULL, the attributes are said to be "available".

From the docs: """ Retrieve the list of available sysattrs, with value being empty; This just return all available sysfs attributes for a particular device without reading their values. """

I dunno what "available" is supposed to mean, but if these attributes are further looked up by "udev_device_get_sysattr_value" they may have no value, i.e., this method may return NULL. (This is not the same as having a value like the empty string, which fits the specification just fine.)

Version-Release number of selected component (if applicable):

Any.

How reproducible:

Consistently, for a given device.

Steps to Reproduce:

  1. Loop over all devices returned by udevadm.
  2. List the attributes using udev_device_get_sysattr_list_entry().
  3. Look up the listed attributes using udev_device_get_sysattr_value(). Observe that some of these attributes are NULL.

Actual results:

Some "available" attributes have no values. Generally speaking, these attributes seem to be symlinks to directories. It is not true that all attributes that correspond to symlinks to directories have no value, only some.

Expected results:

"Available" attributes should have a value or documentation should define clearly that "available" means fits some criteria which does not necessarily include actually having a value.

Additional info:

It is normal to think of attribute name/value pairs as being like keys in a map. If they are not, that is, if a key can simultaneously be in the map and not be in the map, that should get better documentation then by the single word "available".

Also, in documentation, the ';' is followed by a capital letter, should be lowercase.

--- Additional comment from mulhern on 2015-09-22 12:11:50 EDT ---

Output of new.c on my Fedora desktop. This is just an example, similar things happen on my RHEL 7.2 box.

[mulhern@localhost reproducers]$ ./a.out udev_list_entry_get_value() Correct behavior, values are not obtained. adr: (null) path: (null) subsystem: (null) uevent: (null) physical_node: (null)

udev_device_get_sysattr_value() Incorrect behavior; physicalnode should have a value. adr: 0x00000000 path: _SB.PCI0.EHC2.HUBN subsystem: acpi uevent: physical_node: (null)

udev_device_get_sysattr_value(bogus) correct behavior, value of non-existant attribute is null. bogus: (null)

Should be like the first. adr: (null) path: (null) subsystem: (null) uevent: (null) physical_node: (null)

--- Additional comment from mulhern on 2015-09-23 15:35 EDT ---

--- Additional comment from mulhern on 2015-09-23 17:03:36 EDT ---

The attached file printed SURPRISE a few times on both systems, indicating that some of the attributes with no value were not symlinked directories. What characteristics they do have I do not know.

Since attributes inherits from Mapping, values method is defined by the Mapping class using in, like, "[s[key] for key in s]". Due to the libudev inconsistency described in #1265315, this is likely to raise an error when key is both in, in the second use of 'in', but not in in the first use of 'in'.

dcuddeback commented 7 years ago

@mulkieran Isn't that already being handled correctly in this library? See here: https://github.com/dcuddeback/libudev-rs/blob/master/src/device.rs#L337. The iterator yields Attribute structs, which return Option<&OsStr> when asked for the attribute's value.

mulkieran commented 7 years ago

I see! You check for a null-pointer when returning the attribute value. This is good and better than pyudev did originally. What this means is that you can iterate only through a subset of the available attributes, the ones that you get when you invoke, udev_device_get_sysattr_list_entry, but that when getting the values you won't crash, because you return an Option type. pyudev went overboard initially, and extended Mapping, and got itself into conceptual trouble.

I would say, that since, in the general case, attributes exist that are not in the list returned by your attributes() function, the "all" in the header comment is overstating the case. Another way of saying this is that attribute_value() and set_attribute_value() can get and set names that aren't in the result of attributes(). Usually, those names will have "/"s in them, but sometimes not. But that is about the only problem.

dcuddeback commented 7 years ago

Okay, so not a bug then. And the other issue was addressed in another PR, so this ticket can be closed. Thanks.

mulkieran commented 7 years ago

Thanks for surfacing! Now that I know your there I'll start keeping a serious eye out for any other surprising things that have been revealed by all that pyudev testing...these were just the things that looked wrong in five minutes poking around. I think, though, that libudev-rs may have avoided some mistakes that pyudev made by not having inheritance...there's a lesson in there somewhere.