udev patch

Hi there,

Currently udev does not reread changed rule files when using symlinks.
Debian uses the symlink approach resulting to something like this:

020_permissions.rules -> ../permissions.rules
025_libgphoto2.rules -> ../libgphoto2.rules

Because udev uses inotify on the rules directory it cannot detect changes to
the destination file of a link. The attached patch solves this problem by using inotify on the directory where each symlink destination resides.

I've tried using inotify on the destination filenames but this doesn't work
as expected. For example vi will unlink and recreate the files. This means that the symlink will not point anywere for some time which is enough for udev to loose the destination. After that the link needs to be rechecked which is not possible (and efficient).

Please consider the attached patch since it removes the need
of 'udevcontrol reload_rules' when working with symlinks.

If I have to set a license for the patch the it is GPLv2.

Thanks in advance
Harhalakis Stefanos

p.s. CC replies
p.s.2. Be gentle :-)

diff -ru udev-103.orig/udevd.c udev-103/udevd.c
@@ -929,6 +929,100 @@
 	}
 }
 
+static void check_link(const char *filename)
+{
+	struct stat	stats;
+	struct stat	lstats;
+	char		fn[PATH_SIZE+1];
+	char		fn2[PATH_SIZE*2+3];
+	char		*p;
+	ssize_t		sz;
+
+	if (lstat(filename, &lstats) != 0)
+	{
+		err("could not read '%s': %s", filename, strerror(errno));
+		return;
+	}
+
+	/* Check if it is a link */
+	if (!S_ISLNK(lstats.st_mode))
+		return;
+
+	/* Get the destination */
+	dbg("found link: %s", filename);
+	sz=readlink(filename, fn, PATH_SIZE);
+	if (sz>=PATH_SIZE)
+	{
+		err("oversized link destination of '%s'", filename);
+		return;
+	}
+
+	fn[sz]=0;
+
+	/* Form the full destination path */
+	if (fn[0]=='/')
+	{
+		strcpy(fn2, fn);
+	} else
+	{
+		snprintf(fn2, PATH_SIZE*2+2, "%s/%s", udev_rules_filename, fn);
+		fn2[PATH_SIZE*2+2]=0;
+	}
+
+	/* Check if destination if a file */
+	stat(fn2, &stats);
+	if (!S_ISREG(stats.st_mode))
+		return;
+
+	/* Find the parent directory */
+	p=fn2+strlen(fn2);
+
+	while (p>=fn2)
+	{
+		if (*p=='/')
+		{
+			*p=0;
+			break;
+		}
+		p--;
+	}
+
+	if (p==fn2)
+		err("failed to get parrent directory of '%s'", fn2);
+
+	dbg("adding notify for:'%s'", fn2);
+	inotify_add_watch(inotify_fd, fn2, IN_CREATE | IN_DELETE | IN_MOVE | IN_CLOSE_WRITE);
+}
+
+static void start_inotify(void)
+{
+	struct name_entry	*name_loop, *name_tmp;
+
+	/* watch rules directory */
+	inotify_fd = inotify_init();
+	if (inotify_fd >= 0)
+	{
+		inotify_add_watch(inotify_fd, udev_rules_filename, IN_CREATE | IN_DELETE | IN_MOVE | IN_CLOSE_WRITE);
+	} else if (errno == ENOSYS)
+	{
+		err("the kernel does not support inotify, udevd can't monitor configuration file changes");
+		return;
+	} else
+	{
+		err("inotify_init failed: %s", strerror(errno));
+		return;
+	}
+
+	LIST_HEAD(name_list);
+
+	add_matching_files(&name_list, udev_rules_filename, RULEFILE_SUFFIX);
+	list_for_each_entry_safe(name_loop, name_tmp, &name_list, node) {
+		check_link(name_loop->name);
+		list_del(&name_loop->node);
+		free(name_loop);
+	}
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	int retval;

@@ -1101,14 +1195,7 @@
 	sigaction(SIGCHLD, &act, NULL);
 	sigaction(SIGHUP, &act, NULL);
 
-	/* watch rules directory */
-	inotify_fd = inotify_init();
-	if (inotify_fd >= 0)
-		inotify_add_watch(inotify_fd, udev_rules_filename, IN_CREATE | IN_DELETE | IN_MOVE | IN_CLOSE_WRITE);
-	else if (errno == ENOSYS)
-		err("the kernel does not support inotify, udevd can't monitor configuration file changes");
-	else
-		err("inotify_init failed: %s", strerror(errno));
+	start_inotify();
 
 	/* maximum limit of forked childs */
 	value = getenv("UDEVD_MAX_CHILDS");

@@ -1145,15 +1232,15 @@
 	if (debug_trace)
 		putenv("DEBUG=1");
 
-	maxfd = udevd_sock;
-	maxfd = UDEV_MAX(maxfd, uevent_netlink_sock);
-	maxfd = UDEV_MAX(maxfd, signal_pipe[READ_END]);
-	maxfd = UDEV_MAX(maxfd, inotify_fd);
-
 	while (!udev_exit) {
 		struct udevd_uevent_msg *msg;
 		int fdcount;
 
+		maxfd = udevd_sock;
+		maxfd = UDEV_MAX(maxfd, uevent_netlink_sock);
+		maxfd = UDEV_MAX(maxfd, signal_pipe[READ_END]);
+		maxfd = UDEV_MAX(maxfd, inotify_fd);
+
 		FD_ZERO(&readfds);
 		FD_SET(signal_pipe[READ_END], &readfds);
 		FD_SET(udevd_sock, &readfds);

@@ -1209,6 +1296,12 @@
 		/* rules changed, set by inotify or a HUP signal */
 		if (reload_config) {
 			reload_config = 0;
+			/* Restart inotify to handle possible file changes */
+			if (inotify_fd>0)
+			{
+				close(inotify_fd);
+				start_inotify();
+			}
 			udev_rules_cleanup(&rules);
 			udev_rules_init(&rules, 1);
 		}

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

I have often wondered why Debian uses symlinks for the rules files. Is it just for compatibility with an ancient version of udev, that put the rules somewhere else? Or is it something with package upgrades, to handle it better when users decide to remove rules files? (If that's the case, then could the normal dpkg config-file handling -- or anything that wouldn't require the files to be kept in two places -- work?) Or is it something else?

Or has this already been discussed to death on Debian lists? ;-)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

I'm not a debian developer, maintainer or anything more than a debian user,
so I'll just copy & paste a part from the README.Debian of the udev package:

-----
MAINTAINERS BEWARE: the use of /etc/udev/rules.d/ by other packages is discouraged, except when only RUN rules are added.
If you think your package needs to create a file there, then please contact the udev package maintainer and explain your needs. Packages should NEVER create files in /etc/udev/rules.d/, but create a symlink the first time the package is installed (and never try again, to allow the local system administrator to remove it).
-----

A mail that illustrates this can be found at:
http://lists.alioth.debian.org/pipermail/pkg-alsa-devel/2006-March/003158.html

There is a debhelper for udev rules called 'dh_installudev' which says:

-----
DESCRIPTION
dh_installudev is a debhelper program that is responsible for installing udev rules files.

Files named debian/package.udev will be installed as etc/udev/pack‐ age.rules in the package build directory.

Then postinst and postrm commands are automatically generated to enable the rules file when the package is first installed, by creating a sym‐ link to it in the /etc/udev/rules.d/ directory. See dh_installdeb(1) for an explanation of how this works.
-----

So I believe that this matches both of your answers, about better
removal/addition and about the long discussion :-)

I've CC'd this to Marco d'Itri <[email protected]>, which is the debian udev
package maintainer and may answer your questions in more depth.

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Shouldn't this apply to any config directory for any package, e.g. Debian's Apache package's /etc/apache/{mods,sites}-enabled? Yet at least one time that I upgraded Apache, both of those directories were reset, which broke the web server setup that I had. (I believe this was the upgrade from Apache 2.0 to 2.2, but I'm not sure on that.)

But regardless of that, here's another solution that might allow both:
Maybe you could have the sysadmin rename the files they want to disable (to e.g. package.rules.off) so that udev ignores them, and then have the dh_installudev script treat these files like removed symlinks. Then at least inotify would work, without having to lstat the files and figure out which directory their target is in. (Especially when the target is itself a symlink, etc., etc...) Maybe that's not possible, though.

It does, thank you. :-)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Conffiles handling is suboptimal when a file is deleted. *Some* symlinks are needed, to choose the /dev layout, while other nowadays could probably become simple files.
In the beginning I tried to have something as flexible as possible to account for future changes, after the release I will try to remove some complexity.

Anyway, I would like to see this patch merged.

ciao,
Marco

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel