libsensors: No longer depend on libsysfs

Hi all,

Here is a proposed patch to make libsensors no longer depend on libsysfs. Instead, it accesses sysfs directly, using 3 embedded helper functions. My motivations for doing this are:
* As far as I know, libsysfs is no longer maintained.
* libsysfs does much more than we need. For example, when asking for a device attribute list, libsysfs will read the contents and permissions of all attributes. Not only does this waste CPU cycles per se, but in the case of hwmon driver it also triggers register reads, which can be slow for SMBus chips.
* libsysfs enforces the difference between devices and class devices, while future changes will be easier if we can handle both types alike.

Quoted 14 lines · Jean Delvare

Hi all,

Here is a proposed patch to make libsensors no longer depend on libsysfs. Instead, it accesses sysfs directly, using 3 embedded helper functions. My motivations for doing this are:
* As far as I know, libsysfs is no longer maintained.
* libsysfs does much more than we need. For example, when asking for a device attribute list, libsysfs will read the contents and permissions of all attributes. Not only does this waste CPU cycles per se, but in the case of hwmon driver it also triggers register reads, which can be slow for SMBus chips.
* libsysfs enforces the difference between devices and class devices, while future changes will be easier if we can handle both types alike.

Good work!

I'm afraid I don't have the time to review this atm though, any other takers?

Hi Hans,

Quoted 17 lines · Hans de Goede

Quoted 14 lines · Jean Delvare

Hi all,

Here is a proposed patch to make libsensors no longer depend on libsysfs. Instead, it accesses sysfs directly, using 3 embedded helper functions. My motivations for doing this are:
* As far as I know, libsysfs is no longer maintained.
* libsysfs does much more than we need. For example, when asking for a device attribute list, libsysfs will read the contents and permissions of all attributes. Not only does this waste CPU cycles per se, but in the case of hwmon driver it also triggers register reads, which can be slow for SMBus chips.
* libsysfs enforces the difference between devices and class devices, while future changes will be easier if we can handle both types alike.

Good work!

Quoted 1 line

I'm afraid I don't have the time to review this atm though, any other takers?

If you have the possibility to test the patch, that would be appreciated as well, and less time-consuming than a review. Different kernel versions and configurations can make the sysfs tree look a bit different. Also, i2c and non-i2c chips have different code paths. I've tested as much as I could on my systems, but more testing can't hurt.

Quoted 5 lines · Jean Delvare

If you have the possibility to test the patch, that would be appreciated as well, and less time-consuming than a review. Different kernel versions and configurations can make the sysfs tree look a bit different. Also, i2c and non-i2c chips have different code paths. I've tested as much as I could on my systems, but more testing can't hurt.

I've just given it a test run on my abituguru machine, works fine. Tomorrow I'll give it a test run on a FSC machine.

If you're reasonably sure this patch is ok, I could put it in the Fedora Rawhide / devel package for some greater exposure, then we can be pretty sure there will be plenty of testing before we release 3.0.1 .

Well, I have no reason to believe that anything is wrong with this patch, as all my testing has been successful. Feel free to apply it to Rawhide if you want. I would like to commit the change soon, the only thing I am waiting for at the moment is Mark's ack, as we had been discussing the matter before, he might want to comment on the approach I took in my patch.

Quoted 6 lines · Jean Delvare

Well, I have no reason to believe that anything is wrong with this patch, as all my testing has been successful. Feel free to apply it to Rawhide if you want. I would like to commit the change soon, the only thing I am waiting for at the moment is Mark's ack, as we had been discussing the matter before, he might want to comment on the approach I took in my patch.

Patch is committed now.

Hi Jean:

Quoted 9 lines · Jean Delvare

Quoted 6 lines · Jean Delvare

Well, I have no reason to believe that anything is wrong with this patch, as all my testing has been successful. Feel free to apply it to Rawhide if you want. I would like to commit the change soon, the only thing I am waiting for at the moment is Mark's ack, as we had been discussing the matter before, he might want to comment on the approach I took in my patch.

Patch is committed now.

I've finally finished reviewing this. I only have one minor change to suggest, which IMHO makes the code a little more readable at very little expense in cycles and no expense in size.

Index: lib/sysfs.c
=================================> --- lib/sysfs.c (revision 5072)
+++ lib/sysfs.c (working copy)

lib/sysfs.c +8 -8

@@ -73,7 +73,7 @@
  * local errors, or a negative error value if any call fails.
  */
 static int sysfs_foreach_classdev(const char *class_name,
-				   int (*func)(char *, const char*))
+				   int (*func)(const char *, const char *))
 {
 	char path[NAME_MAX];
 	int path_off, ret;

@@ -105,7 +105,7 @@
  * local errors, or a negative error value if any call fails.
  */
 static int sysfs_foreach_busdev(const char *bus_type,
-				 int (*func)(char *, const char*))
+				 int (*func)(const char *, const char *))
 {
 	char path[NAME_MAX];
 	int path_off, ret;

@@ -455,7 +455,7 @@
 }
 
 /* returns: 0 if successful, !0 otherwise */
-static int sensors_read_one_sysfs_chip(char *dev_path, const char *dev_name)
+static int sensors_read_one_sysfs_chip(const char *dev_path, const char *dev_name)
 {
 	int domain, bus, slot, fn;
 	int err = -SENSORS_ERR_KERNEL;

@@ -539,20 +539,20 @@
 	return 0;
 }
 
-static int sensors_add_hwmon_device(char *path, const char *classdev)
+static int sensors_add_hwmon_device(const char *path, const char *classdev)
 {
+	char linkpath[NAME_MAX];
 	char device[NAME_MAX];
-	int path_off = strlen(path);
 	int dev_len;
 	(void)classdev; /* hide warning */
 
-	snprintf(path + path_off, NAME_MAX - path_off, "/device");
-	dev_len = readlink(path, device, NAME_MAX - 1);
+	snprintf(linkpath, NAME_MAX, "%s/device", path);
+	dev_len = readlink(linkpath, device, NAME_MAX - 1);
 	if (dev_len < 0)
 		return -SENSORS_ERR_KERNEL;
 	device[dev_len] = '\0';
 
-	return sensors_read_one_sysfs_chip(path, strrchr(device, '/') + 1);
+	return sensors_read_one_sysfs_chip(linkpath, strrchr(device, '/') + 1);
 }
 
 /* returns 0 if successful, !0 otherwise */

Quoted 3 lines

I've finally finished reviewing this. I only have one minor change to suggest, which IMHO makes the code a little more readable at very little expense in cycles and no expense in size.

I'm totally fine with this patch, I agree that it's somewhat cleaner and the additional cost is negligible. Feel free to apply it to SVN.

Thanks for the review,