[RFC PATCH] Split udev_sysfs.c to leave out caches from libudev

[RFC PATCH] Split udev_sysfs.c to leave out caches from libudev

[Alan Jenkins]

udev_sysfs.c device and attribute value caches are only appropriate for
short-lived processes, like udev-event and usbid.  In long lived processes
they would fill up with stale data.

Fortunately libudev doesn't use these caches.  But this should be made
explicit to guard against future changes.  Let's move the caches into a
separate file, with its own initialization and cleanup functions.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/extras/usb_id/Makefile.am b/extras/usb_id/Makefile.am
index 0de004a..bf3aa1c 100644
--- a/extras/usb_id/Makefile.am
+++ b/extras/usb_id/Makefile.am
@@ -9,6 +9,7 @@ AM_CPPFLAGS = \
 usb_id_SOURCES = \
 	usb_id.c \
 	../../udev/udev_sysfs.c \
+	../../udev/udev_sysfs_cache.c \
 	../../udev/udev_sysdeps.c \
 	../../udev/udev_utils.c \
 	../../udev/udev_utils_string.c
diff --git a/extras/usb_id/usb_id.c b/extras/usb_id/usb_id.c
index 92492c8..cd0705d 100644
--- a/extras/usb_id/usb_id.c
+++ b/extras/usb_id/usb_id.c
@@ -387,6 +387,7 @@ int main(int argc, char **argv)
 
 	logging_init("usb_id");
 	sysfs_init();
+	sysfs_cache_init();
 
 	while (1) {
 		int option;
@@ -463,6 +464,7 @@ int main(int argc, char **argv)
 	}
 
 exit:
+	sysfs_cache_cleanup();
 	sysfs_cleanup();
 	logging_close();
 	return retval;
diff --git a/udev/Makefile.am b/udev/Makefile.am
index 4a7c785..2d25518 100644
--- a/udev/Makefile.am
+++ b/udev/Makefile.am
@@ -30,6 +30,7 @@ common_files = \
 	udev_rules_parse.c \
 	udev_sysdeps.c \
 	udev_sysfs.c \
+	udev_sysfs_cache.c \
 	udev_utils.c \
 	udev_utils_file.c \
 	udev_utils_string.c
diff --git a/udev/test-udev.c b/udev/test-udev.c
index 591e930..484c3c0 100644
--- a/udev/test-udev.c
+++ b/udev/test-udev.c
@@ -130,6 +130,7 @@ int main(int argc, char *argv[], char *envp[])
 	}
 
 	sysfs_init();
+	sysfs_cache_init();
 	udev_rules_init(&rules, 0);
 
 	dev = sysfs_device_get(devpath);
@@ -166,6 +167,7 @@ int main(int argc, char *argv[], char *envp[])
 	udev_device_cleanup(udev);
 fail:
 	udev_rules_cleanup(&rules);
+	sysfs_cache_cleanup();
 	sysfs_cleanup();
 	selinux_exit();
 
diff --git a/udev/udev.h b/udev/udev.h
index b633807..455b68e 100644
--- a/udev/udev.h
+++ b/udev/udev.h
@@ -114,12 +114,16 @@ extern int sysfs_init(void);
 extern void sysfs_cleanup(void);
 extern void sysfs_device_set_values(struct sysfs_device *dev, const char *devpath,
 				    const char *subsystem, const char *driver);
+extern int sysfs_resolve_link(char *path, size_t size);
+extern int sysfs_lookup_devpath_by_subsys_id(char *devpath, size_t len, const char *subsystem, const char *id);
+
+/* udev_sysfs_cache.c */
+extern int sysfs_cache_init(void);
+extern void sysfs_cache_cleanup(void);
 extern struct sysfs_device *sysfs_device_get(const char *devpath);
 extern struct sysfs_device *sysfs_device_get_parent(struct sysfs_device *dev);
 extern struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device *dev, const char *subsystem);
 extern char *sysfs_attr_get_value(const char *devpath, const char *attr_name);
-extern int sysfs_resolve_link(char *path, size_t size);
-extern int sysfs_lookup_devpath_by_subsys_id(char *devpath, size_t len, const char *subsystem, const char *id);
 
 /* udev_node.c */
 extern int udev_node_mknod(struct udevice *udev, const char *file, dev_t devt, mode_t mode, uid_t uid, gid_t gid);
diff --git a/udev/udev_device.c b/udev/udev_device.c
index 9888676..a2f225a 100644
--- a/udev/udev_device.c
+++ b/udev/udev_device.c
@@ -66,17 +66,3 @@ void udev_device_cleanup(struct udevice *udev)
 	name_list_cleanup(&udev->env_list);
 	free(udev);
 }
-
-dev_t udev_device_get_devt(struct udevice *udev)
-{
-	const char *attr;
-	unsigned int maj, min;
-
-	/* read it from sysfs  */
-	attr = sysfs_attr_get_value(udev->dev->devpath, "dev");
-	if (attr != NULL) {
-		if (sscanf(attr, "%u:%u", &maj, &min) = 2)
-			return makedev(maj, min);
-	}
-	return makedev(0, 0);
-}
diff --git a/udev/udev_device_event.c b/udev/udev_device_event.c
index 045035d..d031226 100644
--- a/udev/udev_device_event.c
+++ b/udev/udev_device_event.c
@@ -257,3 +257,17 @@ int udev_device_event(struct udev_rules *rules, struct udevice *udev)
 exit:
 	return retval;
 }
+
+dev_t udev_device_get_devt(struct udevice *udev)
+{
+	const char *attr;
+	unsigned int maj, min;
+
+	/* read it from sysfs  */
+	attr = sysfs_attr_get_value(udev->dev->devpath, "dev");
+	if (attr != NULL) {
+		if (sscanf(attr, "%u:%u", &maj, &min) = 2)
+			return makedev(maj, min);
+	}
+	return makedev(0, 0);
+}
diff --git a/udev/udev_sysfs.c b/udev/udev_sysfs.c
index 91f5997..c4247fa 100644
--- a/udev/udev_sysfs.c
+++ b/udev/udev_sysfs.c
@@ -31,18 +31,6 @@
 
 char sysfs_path[PATH_SIZE];
 
-/* device cache */
-static LIST_HEAD(dev_list);
-
-/* attribute value cache */
-static LIST_HEAD(attr_list);
-struct sysfs_attr {
-	struct list_head node;
-	char path[PATH_SIZE];
-	char *value;			/* points to value_local if value is cached */
-	char value_local[NAME_SIZE];
-};
-
 int sysfs_init(void)
 {
 	const char *env;
@@ -55,27 +43,11 @@ int sysfs_init(void)
 		strlcpy(sysfs_path, "/sys", sizeof(sysfs_path));
 	dbg("sysfs_path='%s'\n", sysfs_path);
 
-	INIT_LIST_HEAD(&dev_list);
-	INIT_LIST_HEAD(&attr_list);
 	return 0;
 }
 
 void sysfs_cleanup(void)
 {
-	struct sysfs_attr *attr_loop;
-	struct sysfs_attr *attr_temp;
-	struct sysfs_device *dev_loop;
-	struct sysfs_device *dev_temp;
-
-	list_for_each_entry_safe(attr_loop, attr_temp, &attr_list, node) {
-		list_del(&attr_loop->node);
-		free(attr_loop);
-	}
-
-	list_for_each_entry_safe(dev_loop, dev_temp, &dev_list, node) {
-		list_del(&dev_loop->node);
-		free(dev_loop);
-	}
 }
 
 void sysfs_device_set_values(struct sysfs_device *dev, const char *devpath,
@@ -144,280 +116,6 @@ int sysfs_resolve_link(char *devpath, size_t size)
 	return 0;
 }
 
-struct sysfs_device *sysfs_device_get(const char *devpath)
-{
-	char path[PATH_SIZE];
-	char devpath_real[PATH_SIZE];
-	struct sysfs_device *dev;
-	struct sysfs_device *dev_loop;
-	struct stat statbuf;
-	char link_path[PATH_SIZE];
-	char link_target[PATH_SIZE];
-	int len;
-	char *pos;
-
-	/* we handle only these devpathes */
-	if (devpath != NULL &&
-	    strncmp(devpath, "/devices/", 9) != 0 &&
-	    strncmp(devpath, "/subsystem/", 11) != 0 &&
-	    strncmp(devpath, "/module/", 8) != 0 &&
-	    strncmp(devpath, "/bus/", 5) != 0 &&
-	    strncmp(devpath, "/class/", 7) != 0 &&
-	    strncmp(devpath, "/block/", 7) != 0)
-		return NULL;
-
-	dbg("open '%s'\n", devpath);
-	strlcpy(devpath_real, devpath, sizeof(devpath_real));
-	remove_trailing_chars(devpath_real, '/');
-	if (devpath[0] = '\0' )
-		return NULL;
-
-	/* look for device already in cache (we never put an untranslated path in the cache) */
-	list_for_each_entry(dev_loop, &dev_list, node) {
-		if (strcmp(dev_loop->devpath, devpath_real) = 0) {
-			dbg("found in cache '%s'\n", dev_loop->devpath);
-			return dev_loop;
-		}
-	}
-
-	/* if we got a link, resolve it to the real device */
-	strlcpy(path, sysfs_path, sizeof(path));
-	strlcat(path, devpath_real, sizeof(path));
-	if (lstat(path, &statbuf) != 0) {
-		dbg("stat '%s' failed: %s\n", path, strerror(errno));
-		return NULL;
-	}
-	if (S_ISLNK(statbuf.st_mode)) {
-		if (sysfs_resolve_link(devpath_real, sizeof(devpath_real)) != 0)
-			return NULL;
-
-		/* now look for device in cache after path translation */
-		list_for_each_entry(dev_loop, &dev_list, node) {
-			if (strcmp(dev_loop->devpath, devpath_real) = 0) {
-				dbg("found in cache '%s'\n", dev_loop->devpath);
-				return dev_loop;
-			}
-		}
-	}
-
-	/* it is a new device */
-	dbg("new uncached device '%s'\n", devpath_real);
-	dev = malloc(sizeof(struct sysfs_device));
-	if (dev = NULL)
-		return NULL;
-	memset(dev, 0x00, sizeof(struct sysfs_device));
-
-	sysfs_device_set_values(dev, devpath_real, NULL, NULL);
-
-	/* get subsystem name */
-	strlcpy(link_path, sysfs_path, sizeof(link_path));
-	strlcat(link_path, dev->devpath, sizeof(link_path));
-	strlcat(link_path, "/subsystem", sizeof(link_path));
-	len = readlink(link_path, link_target, sizeof(link_target));
-	if (len > 0) {
-		/* get subsystem from "subsystem" link */
-		link_target[len] = '\0';
-		dbg("subsystem link '%s' points to '%s'\n", link_path, link_target);
-		pos = strrchr(link_target, '/');
-		if (pos != NULL)
-			strlcpy(dev->subsystem, &pos[1], sizeof(dev->subsystem));
-	} else if (strstr(dev->devpath, "/drivers/") != NULL) {
-		strlcpy(dev->subsystem, "drivers", sizeof(dev->subsystem));
-	} else if (strncmp(dev->devpath, "/module/", 8) = 0) {
-		strlcpy(dev->subsystem, "module", sizeof(dev->subsystem));
-	} else if (strncmp(dev->devpath, "/subsystem/", 11) = 0) {
-		pos = strrchr(dev->devpath, '/');
-		if (pos = &dev->devpath[10])
-			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
-	} else if (strncmp(dev->devpath, "/class/", 7) = 0) {
-		pos = strrchr(dev->devpath, '/');
-		if (pos = &dev->devpath[6])
-			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
-	} else if (strncmp(dev->devpath, "/bus/", 5) = 0) {
-		pos = strrchr(dev->devpath, '/');
-		if (pos = &dev->devpath[4])
-			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
-	}
-
-	/* get driver name */
-	strlcpy(link_path, sysfs_path, sizeof(link_path));
-	strlcat(link_path, dev->devpath, sizeof(link_path));
-	strlcat(link_path, "/driver", sizeof(link_path));
-	len = readlink(link_path, link_target, sizeof(link_target));
-	if (len > 0) {
-		link_target[len] = '\0';
-		dbg("driver link '%s' points to '%s'\n", link_path, link_target);
-		pos = strrchr(link_target, '/');
-		if (pos != NULL)
-			strlcpy(dev->driver, &pos[1], sizeof(dev->driver));
-	}
-
-	dbg("add to cache 'devpath=%s', subsystem='%s', driver='%s'\n", dev->devpath, dev->subsystem, dev->driver);
-	list_add(&dev->node, &dev_list);
-
-	return dev;
-}
-
-struct sysfs_device *sysfs_device_get_parent(struct sysfs_device *dev)
-{
-	char parent_devpath[PATH_SIZE];
-	char *pos;
-
-	dbg("open '%s'\n", dev->devpath);
-
-	/* look if we already know the parent */
-	if (dev->parent != NULL)
-		return dev->parent;
-
-	strlcpy(parent_devpath, dev->devpath, sizeof(parent_devpath));
-	dbg("'%s'\n", parent_devpath);
-
-	/* strip last element */
-	pos = strrchr(parent_devpath, '/');
-	if (pos = NULL || pos = parent_devpath)
-		return NULL;
-	pos[0] = '\0';
-
-	if (strncmp(parent_devpath, "/class", 6) = 0) {
-		pos = strrchr(parent_devpath, '/');
-		if (pos = &parent_devpath[6] || pos = parent_devpath) {
-			dbg("/class top level, look for device link\n");
-			goto device_link;
-		}
-	}
-	if (strcmp(parent_devpath, "/block") = 0) {
-		dbg("/block top level, look for device link\n");
-		goto device_link;
-	}
-
-	/* are we at the top level? */
-	pos = strrchr(parent_devpath, '/');
-	if (pos = NULL || pos = parent_devpath)
-		return NULL;
-
-	/* get parent and remember it */
-	dev->parent = sysfs_device_get(parent_devpath);
-	return dev->parent;
-
-device_link:
-	strlcpy(parent_devpath, dev->devpath, sizeof(parent_devpath));
-	strlcat(parent_devpath, "/device", sizeof(parent_devpath));
-	if (sysfs_resolve_link(parent_devpath, sizeof(parent_devpath)) != 0)
-		return NULL;
-
-	/* get parent and remember it */
-	dev->parent = sysfs_device_get(parent_devpath);
-	return dev->parent;
-}
-
-struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device *dev, const char *subsystem)
-{
-	struct sysfs_device *dev_parent;
-
-	dev_parent = sysfs_device_get_parent(dev);
-	while (dev_parent != NULL) {
-		if (strcmp(dev_parent->subsystem, subsystem) = 0)
-			return dev_parent;
-		dev_parent = sysfs_device_get_parent(dev_parent);
-	}
-	return NULL;
-}
-
-char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
-{
-	char path_full[PATH_SIZE];
-	const char *path;
-	char value[NAME_SIZE];
-	struct sysfs_attr *attr_loop;
-	struct sysfs_attr *attr;
-	struct stat statbuf;
-	int fd;
-	ssize_t size;
-	size_t sysfs_len;
-
-	dbg("open '%s'/'%s'\n", devpath, attr_name);
-	sysfs_len = strlcpy(path_full, sysfs_path, sizeof(path_full));
-	if(sysfs_len >= sizeof(path_full))
-		sysfs_len = sizeof(path_full) - 1;
-	path = &path_full[sysfs_len];
-	strlcat(path_full, devpath, sizeof(path_full));
-	strlcat(path_full, "/", sizeof(path_full));
-	strlcat(path_full, attr_name, sizeof(path_full));
-
-	/* look for attribute in cache */
-	list_for_each_entry(attr_loop, &attr_list, node) {
-		if (strcmp(attr_loop->path, path) = 0) {
-			dbg("found in cache '%s'\n", attr_loop->path);
-			return attr_loop->value;
-		}
-	}
-
-	/* store attribute in cache (also negatives are kept in cache) */
-	dbg("new uncached attribute '%s'\n", path_full);
-	attr = malloc(sizeof(struct sysfs_attr));
-	if (attr = NULL)
-		return NULL;
-	memset(attr, 0x00, sizeof(struct sysfs_attr));
-	strlcpy(attr->path, path, sizeof(attr->path));
-	dbg("add to cache '%s'\n", path_full);
-	list_add(&attr->node, &attr_list);
-
-	if (lstat(path_full, &statbuf) != 0) {
-		dbg("stat '%s' failed: %s\n", path_full, strerror(errno));
-		goto out;
-	}
-
-	if (S_ISLNK(statbuf.st_mode)) {
-		/* links return the last element of the target path */
-		char link_target[PATH_SIZE];
-		int len;
-		const char *pos;
-
-		len = readlink(path_full, link_target, sizeof(link_target));
-		if (len > 0) {
-			link_target[len] = '\0';
-			pos = strrchr(link_target, '/');
-			if (pos != NULL) {
-				dbg("cache '%s' with link value '%s'\n", path_full, value);
-				strlcpy(attr->value_local, &pos[1], sizeof(attr->value_local));
-				attr->value = attr->value_local;
-			}
-		}
-		goto out;
-	}
-
-	/* skip directories */
-	if (S_ISDIR(statbuf.st_mode))
-		goto out;
-
-	/* skip non-readable files */
-	if ((statbuf.st_mode & S_IRUSR) = 0)
-		goto out;
-
-	/* read attribute value */
-	fd = open(path_full, O_RDONLY);
-	if (fd < 0) {
-		dbg("attribute '%s' can not be opened\n", path_full);
-		goto out;
-	}
-	size = read(fd, value, sizeof(value));
-	close(fd);
-	if (size < 0)
-		goto out;
-	if (size = sizeof(value))
-		goto out;
-
-	/* got a valid value, store and return it */
-	value[size] = '\0';
-	remove_trailing_chars(value, '\n');
-	dbg("cache '%s' with attribute value '%s'\n", path_full, value);
-	strlcpy(attr->value_local, value, sizeof(attr->value_local));
-	attr->value = attr->value_local;
-
-out:
-	return attr->value;
-}
-
 int sysfs_lookup_devpath_by_subsys_id(char *devpath_full, size_t len, const char *subsystem, const char *id)
 {
 	size_t sysfs_len;
diff --git a/udev/udev_sysfs_cache.c b/udev/udev_sysfs_cache.c
new file mode 100644
index 0000000..2f33ed5
--- /dev/null
+++ b/udev/udev_sysfs_cache.c
@@ -0,0 +1,341 @@
+/*
+ * Copyright (C) 2005-2006 Kay Sievers <kay.sievers@vrfy.org>
+ *
+ *	This program is free software; you can redistribute it and/or modify it
+ *	under the terms of the GNU General Public License as published by the
+ *	Free Software Foundation version 2 of the License.
+ * 
+ *	This program is distributed in the hope that it will be useful, but
+ *	WITHOUT ANY WARRANTY; without even the implied warranty of
+ *	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *	General Public License for more details.
+ * 
+ *	You should have received a copy of the GNU General Public License along
+ *	with this program; if not, write to the Free Software Foundation, Inc.,
+ *	51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <stddef.h>
+#include <unistd.h>
+#include <string.h>
+#include <fcntl.h>
+#include <ctype.h>
+#include <errno.h>
+#include <sys/stat.h>
+
+#include "udev.h"
+
+/* device cache */
+static LIST_HEAD(dev_list);
+
+/* attribute value cache */
+static LIST_HEAD(attr_list);
+struct sysfs_attr {
+	struct list_head node;
+	char path[PATH_SIZE];
+	char *value;			/* points to value_local if value is cached */
+	char value_local[NAME_SIZE];
+};
+
+int sysfs_cache_init(void)
+{
+	INIT_LIST_HEAD(&dev_list);
+	INIT_LIST_HEAD(&attr_list);
+	return 0;
+}
+
+void sysfs_cache_cleanup(void)
+{
+	struct sysfs_attr *attr_loop;
+	struct sysfs_attr *attr_temp;
+	struct sysfs_device *dev_loop;
+	struct sysfs_device *dev_temp;
+
+	list_for_each_entry_safe(attr_loop, attr_temp, &attr_list, node) {
+		list_del(&attr_loop->node);
+		free(attr_loop);
+	}
+
+	list_for_each_entry_safe(dev_loop, dev_temp, &dev_list, node) {
+		list_del(&dev_loop->node);
+		free(dev_loop);
+	}
+}
+
+struct sysfs_device *sysfs_device_get(const char *devpath)
+{
+	char path[PATH_SIZE];
+	char devpath_real[PATH_SIZE];
+	struct sysfs_device *dev;
+	struct sysfs_device *dev_loop;
+	struct stat statbuf;
+	char link_path[PATH_SIZE];
+	char link_target[PATH_SIZE];
+	int len;
+	char *pos;
+
+	/* we handle only these devpathes */
+	if (devpath != NULL &&
+	    strncmp(devpath, "/devices/", 9) != 0 &&
+	    strncmp(devpath, "/subsystem/", 11) != 0 &&
+	    strncmp(devpath, "/module/", 8) != 0 &&
+	    strncmp(devpath, "/bus/", 5) != 0 &&
+	    strncmp(devpath, "/class/", 7) != 0 &&
+	    strncmp(devpath, "/block/", 7) != 0)
+		return NULL;
+
+	dbg("open '%s'\n", devpath);
+	strlcpy(devpath_real, devpath, sizeof(devpath_real));
+	remove_trailing_chars(devpath_real, '/');
+	if (devpath[0] = '\0' )
+		return NULL;
+
+	/* look for device already in cache (we never put an untranslated path in the cache) */
+	list_for_each_entry(dev_loop, &dev_list, node) {
+		if (strcmp(dev_loop->devpath, devpath_real) = 0) {
+			dbg("found in cache '%s'\n", dev_loop->devpath);
+			return dev_loop;
+		}
+	}
+
+	/* if we got a link, resolve it to the real device */
+	strlcpy(path, sysfs_path, sizeof(path));
+	strlcat(path, devpath_real, sizeof(path));
+	if (lstat(path, &statbuf) != 0) {
+		dbg("stat '%s' failed: %s\n", path, strerror(errno));
+		return NULL;
+	}
+	if (S_ISLNK(statbuf.st_mode)) {
+		if (sysfs_resolve_link(devpath_real, sizeof(devpath_real)) != 0)
+			return NULL;
+
+		/* now look for device in cache after path translation */
+		list_for_each_entry(dev_loop, &dev_list, node) {
+			if (strcmp(dev_loop->devpath, devpath_real) = 0) {
+				dbg("found in cache '%s'\n", dev_loop->devpath);
+				return dev_loop;
+			}
+		}
+	}
+
+	/* it is a new device */
+	dbg("new uncached device '%s'\n", devpath_real);
+	dev = malloc(sizeof(struct sysfs_device));
+	if (dev = NULL)
+		return NULL;
+	memset(dev, 0x00, sizeof(struct sysfs_device));
+
+	sysfs_device_set_values(dev, devpath_real, NULL, NULL);
+
+	/* get subsystem name */
+	strlcpy(link_path, sysfs_path, sizeof(link_path));
+	strlcat(link_path, dev->devpath, sizeof(link_path));
+	strlcat(link_path, "/subsystem", sizeof(link_path));
+	len = readlink(link_path, link_target, sizeof(link_target));
+	if (len > 0) {
+		/* get subsystem from "subsystem" link */
+		link_target[len] = '\0';
+		dbg("subsystem link '%s' points to '%s'\n", link_path, link_target);
+		pos = strrchr(link_target, '/');
+		if (pos != NULL)
+			strlcpy(dev->subsystem, &pos[1], sizeof(dev->subsystem));
+	} else if (strstr(dev->devpath, "/drivers/") != NULL) {
+		strlcpy(dev->subsystem, "drivers", sizeof(dev->subsystem));
+	} else if (strncmp(dev->devpath, "/module/", 8) = 0) {
+		strlcpy(dev->subsystem, "module", sizeof(dev->subsystem));
+	} else if (strncmp(dev->devpath, "/subsystem/", 11) = 0) {
+		pos = strrchr(dev->devpath, '/');
+		if (pos = &dev->devpath[10])
+			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
+	} else if (strncmp(dev->devpath, "/class/", 7) = 0) {
+		pos = strrchr(dev->devpath, '/');
+		if (pos = &dev->devpath[6])
+			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
+	} else if (strncmp(dev->devpath, "/bus/", 5) = 0) {
+		pos = strrchr(dev->devpath, '/');
+		if (pos = &dev->devpath[4])
+			strlcpy(dev->subsystem, "subsystem", sizeof(dev->subsystem));
+	}
+
+	/* get driver name */
+	strlcpy(link_path, sysfs_path, sizeof(link_path));
+	strlcat(link_path, dev->devpath, sizeof(link_path));
+	strlcat(link_path, "/driver", sizeof(link_path));
+	len = readlink(link_path, link_target, sizeof(link_target));
+	if (len > 0) {
+		link_target[len] = '\0';
+		dbg("driver link '%s' points to '%s'\n", link_path, link_target);
+		pos = strrchr(link_target, '/');
+		if (pos != NULL)
+			strlcpy(dev->driver, &pos[1], sizeof(dev->driver));
+	}
+
+	dbg("add to cache 'devpath=%s', subsystem='%s', driver='%s'\n", dev->devpath, dev->subsystem, dev->driver);
+	list_add(&dev->node, &dev_list);
+
+	return dev;
+}
+
+struct sysfs_device *sysfs_device_get_parent(struct sysfs_device *dev)
+{
+	char parent_devpath[PATH_SIZE];
+	char *pos;
+
+	dbg("open '%s'\n", dev->devpath);
+
+	/* look if we already know the parent */
+	if (dev->parent != NULL)
+		return dev->parent;
+
+	strlcpy(parent_devpath, dev->devpath, sizeof(parent_devpath));
+	dbg("'%s'\n", parent_devpath);
+
+	/* strip last element */
+	pos = strrchr(parent_devpath, '/');
+	if (pos = NULL || pos = parent_devpath)
+		return NULL;
+	pos[0] = '\0';
+
+	if (strncmp(parent_devpath, "/class", 6) = 0) {
+		pos = strrchr(parent_devpath, '/');
+		if (pos = &parent_devpath[6] || pos = parent_devpath) {
+			dbg("/class top level, look for device link\n");
+			goto device_link;
+		}
+	}
+	if (strcmp(parent_devpath, "/block") = 0) {
+		dbg("/block top level, look for device link\n");
+		goto device_link;
+	}
+
+	/* are we at the top level? */
+	pos = strrchr(parent_devpath, '/');
+	if (pos = NULL || pos = parent_devpath)
+		return NULL;
+
+	/* get parent and remember it */
+	dev->parent = sysfs_device_get(parent_devpath);
+	return dev->parent;
+
+device_link:
+	strlcpy(parent_devpath, dev->devpath, sizeof(parent_devpath));
+	strlcat(parent_devpath, "/device", sizeof(parent_devpath));
+	if (sysfs_resolve_link(parent_devpath, sizeof(parent_devpath)) != 0)
+		return NULL;
+
+	/* get parent and remember it */
+	dev->parent = sysfs_device_get(parent_devpath);
+	return dev->parent;
+}
+
+struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device *dev, const char *subsystem)
+{
+	struct sysfs_device *dev_parent;
+
+	dev_parent = sysfs_device_get_parent(dev);
+	while (dev_parent != NULL) {
+		if (strcmp(dev_parent->subsystem, subsystem) = 0)
+			return dev_parent;
+		dev_parent = sysfs_device_get_parent(dev_parent);
+	}
+	return NULL;
+}
+
+char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
+{
+	char path_full[PATH_SIZE];
+	const char *path;
+	char value[NAME_SIZE];
+	struct sysfs_attr *attr_loop;
+	struct sysfs_attr *attr;
+	struct stat statbuf;
+	int fd;
+	ssize_t size;
+	size_t sysfs_len;
+
+	dbg("open '%s'/'%s'\n", devpath, attr_name);
+	sysfs_len = strlcpy(path_full, sysfs_path, sizeof(path_full));
+	if(sysfs_len >= sizeof(path_full))
+		sysfs_len = sizeof(path_full) - 1;
+	path = &path_full[sysfs_len];
+	strlcat(path_full, devpath, sizeof(path_full));
+	strlcat(path_full, "/", sizeof(path_full));
+	strlcat(path_full, attr_name, sizeof(path_full));
+
+	/* look for attribute in cache */
+	list_for_each_entry(attr_loop, &attr_list, node) {
+		if (strcmp(attr_loop->path, path) = 0) {
+			dbg("found in cache '%s'\n", attr_loop->path);
+			return attr_loop->value;
+		}
+	}
+
+	/* store attribute in cache (also negatives are kept in cache) */
+	dbg("new uncached attribute '%s'\n", path_full);
+	attr = malloc(sizeof(struct sysfs_attr));
+	if (attr = NULL)
+		return NULL;
+	memset(attr, 0x00, sizeof(struct sysfs_attr));
+	strlcpy(attr->path, path, sizeof(attr->path));
+	dbg("add to cache '%s'\n", path_full);
+	list_add(&attr->node, &attr_list);
+
+	if (lstat(path_full, &statbuf) != 0) {
+		dbg("stat '%s' failed: %s\n", path_full, strerror(errno));
+		goto out;
+	}
+
+	if (S_ISLNK(statbuf.st_mode)) {
+		/* links return the last element of the target path */
+		char link_target[PATH_SIZE];
+		int len;
+		const char *pos;
+
+		len = readlink(path_full, link_target, sizeof(link_target));
+		if (len > 0) {
+			link_target[len] = '\0';
+			pos = strrchr(link_target, '/');
+			if (pos != NULL) {
+				dbg("cache '%s' with link value '%s'\n", path_full, value);
+				strlcpy(attr->value_local, &pos[1], sizeof(attr->value_local));
+				attr->value = attr->value_local;
+			}
+		}
+		goto out;
+	}
+
+	/* skip directories */
+	if (S_ISDIR(statbuf.st_mode))
+		goto out;
+
+	/* skip non-readable files */
+	if ((statbuf.st_mode & S_IRUSR) = 0)
+		goto out;
+
+	/* read attribute value */
+	fd = open(path_full, O_RDONLY);
+	if (fd < 0) {
+		dbg("attribute '%s' can not be opened\n", path_full);
+		goto out;
+	}
+	size = read(fd, value, sizeof(value));
+	close(fd);
+	if (size < 0)
+		goto out;
+	if (size = sizeof(value))
+		goto out;
+
+	/* got a valid value, store and return it */
+	value[size] = '\0';
+	remove_trailing_chars(value, '\n');
+	dbg("cache '%s' with attribute value '%s'\n", path_full, value);
+	strlcpy(attr->value_local, value, sizeof(attr->value_local));
+	attr->value = attr->value_local;
+
+out:
+	return attr->value;
+}
diff --git a/udev/udevd.c b/udev/udevd.c
index 22d261f..bbe0b6d 100644
--- a/udev/udevd.c
+++ b/udev/udevd.c
@@ -1077,6 +1077,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	/* parse the rules and keep them in memory */
 	sysfs_init();
+	sysfs_cache_init();
 	udev_rules_init(&rules, 1);
 
 	export_initial_seqnum();
@@ -1289,6 +1290,7 @@ int main(int argc, char *argv[], char *envp[])
 exit:
 	udev_rules_cleanup(&rules);
 	sysfs_cleanup();
+	sysfs_cache_cleanup();
 	selinux_exit();
 
 	if (signal_pipe[READ_END] >= 0)

Re: [RFC PATCH] Split udev_sysfs.c to leave out caches from libudev

[Kay Sievers]

On Tue, Sep 2, 2008 at 13:09, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> udev_sysfs.c device and attribute value caches are only appropriate for
> short-lived processes, like udev-event and usbid.  In long lived processes
> they would fill up with stale data.

Sure, the cache was never coded to be used in threaded programs.

> Fortunately libudev doesn't use these caches.

For good reason, yeah. And it will not, at least not in its current state.

> But this should be made
> explicit to guard against future changes.  Let's move the caches into a
> separate file, with its own initialization and cleanup functions.

Wouldn't it be nicer to create a sysfs_cache object, which is passed
to all these functions? The "cheap" tools can just use NULL there, but
any possible user could have more than one instance of the cache. The
cache is really needed if you do thousand of rule compares against a
sysfs value, over and over. I guess we should start changing the stuff
to work with local vars, instead of moving the current global stuff to
a different file?

In the long run libudev will take over a lot of the functionality and
replace the old code, but depending on the actual need, it might take
a while ...

Thanks,
Kay

Re: [RFC PATCH] Split udev_sysfs.c to leave out caches from libudev

[Alan Jenkins]

Kay Sievers wrote:
> On Tue, Sep 2, 2008 at 13:09, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> udev_sysfs.c device and attribute value caches are only appropriate for
>> short-lived processes, like udev-event and usbid.  In long lived processes
>> they would fill up with stale data.
>>     
>
> Sure, the cache was never coded to be used in threaded programs.
>
>   
>> Fortunately libudev doesn't use these caches.
>>     
>
> For good reason, yeah. And it will not, at least not in its current state.
>
>   
>> But this should be made
>> explicit to guard against future changes.  Let's move the caches into a
>> separate file, with its own initialization and cleanup functions.
>>     
>
> Wouldn't it be nicer to create a sysfs_cache object, which is passed
> to all these functions? The "cheap" tools can just use NULL there, but
> any possible user could have more than one instance of the cache. The
> cache is really needed if you do thousand of rule compares against a
> sysfs value, over and over. I guess we should start changing the stuff
> to work with local vars, instead of moving the current global stuff to
> a different file?
>   

Good point.  That helps make the cache usage more explicit, and it's
much nicer for my purposes as well.  (I had blindly converted the global
cache variables to per-thread variables, without touching the callers).

Ok.  To keep the existing behaviour where udev_rules_get_run() reuses
the cache from udev_rules_get_name(), they both need a sysfs_cache
parameter.  Then I have to change test-udev and udevtest as well as
udevd - no point having test programs if they use different code paths. 
I'll see what that looks like then and resend.

Thanks
Alan

Re: [RFC PATCH] Split udev_sysfs.c to leave out caches from libudev

[Alan Jenkins]

Alan Jenkins wrote:
> Kay Sievers wrote:
>   
>> On Tue, Sep 2, 2008 at 13:09, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>>   
>>     
>>> udev_sysfs.c device and attribute value caches are only appropriate for
>>> short-lived processes, like udev-event and usbid.  In long lived processes
>>> they would fill up with stale data.
>>>     
>>>       
>> Sure, the cache was never coded to be used in threaded programs.
>>
>>   
>>     
>>> Fortunately libudev doesn't use these caches.
>>>     
>>>       
>> For good reason, yeah. And it will not, at least not in its current state.
>>
>>   
>>     
>>> But this should be made
>>> explicit to guard against future changes.  Let's move the caches into a
>>> separate file, with its own initialization and cleanup functions.
>>>     
>>>       
>> Wouldn't it be nicer to create a sysfs_cache object, which is passed
>> to all these functions? The "cheap" tools can just use NULL there, but
>> any possible user could have more than one instance of the cache. The
>> cache is really needed if you do thousand of rule compares against a
>> sysfs value, over and over. I guess we should start changing the stuff
>> to work with local vars, instead of moving the current global stuff to
>> a different file?
>>   
>>     
>
> Good point.  That helps make the cache usage more explicit, and it's
> much nicer for my purposes as well.  (I had blindly converted the global
> cache variables to per-thread variables, without touching the callers).
>
> Ok.  To keep the existing behaviour where udev_rules_get_run() reuses
> the cache from udev_rules_get_name(), they both need a sysfs_cache
> parameter.  Then I have to change test-udev and udevtest as well as
> udevd - no point having test programs if they use different code paths. 
> I'll see what that looks like then and resend.
>   
Oops.  sysfs_cache can't be made optional ("just use NULL") without a
bit more effort.  At the moment we rely on sysfs_cleanup() freeing
everything in the cache.  If the cache isn't used, we need a new API to
free individual results.

So we also need sysfs_device_cleanup(), to free the device if it's not
on a cache list.  And sysfs_attr_get_value() can copy the result to a
buffer instead of returning a string.  I think that's reasonable.

Alan