On Sun, 29 Jul 2007, Hans de Goede wrote: > I just received this bug report: > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=250013 > > This is caused because the thinkpad_acpi driver now exports its sensors > through the standard hwmon interface, while libsensors does not know how to > handle them (for now). Yeah. I had to choose between adding support for current libsensors, and improving hot key support a great deal in thinkpad-acpi. Since AFAIK the forthcoming new libsensors already knows how to deal with generic hwmon interfaces, I decided to priorize work on the hot key support. > As already said in various posts I'll be away on vacation for 6 days, after > that I will start writing libsensors support for the thinkpad_acpi "chip" > to resolve this issue. If anyone starts work on this, or has already > started please let me know ASAP to stop us from doing double work. Feel free to add support to libsensors 2.x for thinkpad-acpi, I don't believe anyone else is working on it. At least, nobody told me about it, yet. If 3.x needs some thinkpad-acpi specific stuff, feel free to do it as well. You can probably do it better than I do, I don't know lm-sensors that well. And if you have any doubts about something in thinkpad-acpi, or if you find any bugs in the thinkpad-acpi side, drop me a note and I will be on it as soon as possible. > Also a word to the person implementing these changes in the thinkpad_acpi That would be me. > driver, the conversion to the standard hwmon interface is a good thing! But > you should have kept the old interface for backward compatibility. You > cannot just go and remove kernel <-> userland interfaces which have been in > active use for quite some time! Well... There is a reason why I want procfs support gone as soon as pratical, and code like what is inside gnome sensors-applet is it. Let's have a look at it, shall we? (from sensors-applet-1.8.1/src/ibm-acpi-sensors-interface.c): [...] fscanf(fp, "%*[^\n]"); /* Skip to the End of the Line */ fscanf(fp, "%*1[\n]"); /* Skip One Newline */ if (fscanf(fp, "speed: %d", &fan_speed) != 1) { (error stuff) [...] Is the above an acceptable way to parse a key: value list of records inside a file? The order of the records has never been guaranteed anywhere, and in fact, it *did change* a little depending on thinkpad model. All in all, whomever wrote that just took a BIG shortcut to parsing key: value files, instead of looking after the key he wanted, skipping all the blanks, and getting the data he needed (which is something all people using shell scripts got right. Go figure!). Apparently, the applet also behaves very badly in error conditions (as your bug reporter found out) instead of doing something sensible like showing "N/A" in the field it couldn't read. Whomever wrote that code never bothered asking anyone about the interface AFAIK, or even bothered to read the kernel driver code. From a fast look over the code, it looks like it never handled properly the full range of procfs output ibm-acpi could generate, either. Also, properly written stuff that talks to ibm-acpi procfs already deals with missing procfs files in an intelligent way: ibm-acpi used to add new ones from time to time, so you had to always consider that they might not be there if the user was running an older kernel. The application should have shown "N/A" (for not available) or somesuch if a file it wanted was not available in procfs. It's like trying to read a sysfs attribute, and never expecting any errors (EIO, ENXIO, EBUSY, EAGAIN, whatever) back. It *will* eventually break. Badly. > Luckily this bug ended up in my mailbox and know whats going on as I'm an > lm_sensors developer, but this is sure to be also causing problems > elsewhere!! I suppose so. As for it causing problems elsewhere, I didn't get any reports of other stuff breaking. So far. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh
