[patch] udevd

* [patch] udevd - cleanup and better timeout handling
@ 2004-01-25 20:03 Kay Sievers
  2004-01-26 18:22 ` Greg KH
                   ` (30 more replies)
  0 siblings, 31 replies; 32+ messages in thread
From: Kay Sievers @ 2004-01-25 20:03 UTC (permalink / raw)
  To: linux-hotplug

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

Here is the next revision for udevd:
  o Small cleanups all over the place.
  o Swich to the usual linked list format "list.h".
  o Better timeout handling.
      We store a timestamp in in every queued event, so we don't wait longer
      than the timeout specified, if the hole in the list is not shrinking.
  o ignore udevd target if klibc is used

The code is getting better, but we have still major flaws:

  1. We are much too slow.
     We want to exec the  real udev in the background, but a 'remove'
     is much much faster than a 'add', so we have a problem.
     Question: is it neccessary to order events for different devpath's?
     If no, we may wait_pid() for the exec only if we have another udev
     working on the same devpath.

  2. Which sequence is the first one?
     The automatic exit of the daemon, if we have nothing to do, has the
     disadvantage of not knowing if the first incoming seqnum is really
     the first one.
     So we need to delay the exec of the first exec a small amount of time,
     to see if we get one with a smaller seqnum  to exec before.
     Or we simply never exit the daemon, and delay only the very first exec
     after the start of the daemon.
     Which way to go?

  3. Switch away from ancient ipc to sockets?
     We may want threads for this, but klibc doesn't support it.
     Nevermind, the ipc stuff is also not supported :)
     Any idea?

thanks,
Kay

[-- Attachment #2: 01-udevd.patch --]
[-- Type: text/plain, Size: 11477 bytes --]

diff -Nru a/Makefile b/Makefile

--- a/Makefile	Sun Jan 25 20:57:21 2004
+++ b/Makefile	Sun Jan 25 20:57:21 2004
@@ -147,17 +147,19 @@
 		-D__KLIBC__ -fno-builtin-printf
 	LIB_OBJS =
 	LDFLAGS = --static --nostdlib -nostartfiles -nodefaultlibs
+	UDEVD =
 else
 	CRT0 =
 	LIBC = 
 	CFLAGS += -I$(GCCINCDIR)
 	LIB_OBJS = -lc
 	LDFLAGS =
+	UDEVD = $(DAEMON) $(SENDER)
 endif
 
 CFLAGS += -I$(PWD)/libsysfs
 
-all: $(ROOT) $(DAEMON) $(SENDER)
+all: $(ROOT) $(UDEVD)
 	@extras="$(EXTRAS)" ; for target in $$extras ; do \
 		echo $$target ; \
 		$(MAKE) prefix=$(prefix) LD="$(LD)" SYSFS="$(SYSFS)" \
@@ -231,11 +233,11 @@
 	$(LD) $(LDFLAGS) -o $(ROOT) $(CRT0) $(OBJS) $(LIB_OBJS) $(ARCH_LIB_OBJS)
 	$(STRIPCMD) $(ROOT)
 
-$(DAEMON): $(ROOT) udevd.h udevd.o
+$(DAEMON): udevd.h udevd.o udevd.o logging.o
 	$(LD) $(LDFLAGS) -o $(DAEMON) $(CRT0) udevd.o logging.o $(LIB_OBJS) $(ARCH_LIB_OBJS)
 	$(STRIPCMD) $(ROOT)
 
-$(SENDER): $(ROOT) udevd.h udevsend.o
+$(SENDER): udevd.h udevsend.o udevd.o logging.o
 	$(LD) $(LDFLAGS) -o $(SENDER) $(CRT0) udevsend.o logging.o $(LIB_OBJS) $(ARCH_LIB_OBJS)
 	$(STRIPCMD) $(ROOT)
 
diff -Nru a/udevd.c b/udevd.c
--- a/udevd.c	Sun Jan 25 20:57:21 2004
+++ b/udevd.c	Sun Jan 25 20:57:21 2004
@@ -22,6 +22,7 @@
  *
  */
 
+#include <stddef.h>
 #include <sys/types.h>
 #include <sys/ipc.h>
 #include <sys/wait.h>
@@ -32,29 +33,38 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 #include <fcntl.h>
 
+#include "list.h"
 #include "udev.h"
 #include "udevd.h"
 #include "logging.h"
 
 #define BUFFER_SIZE			1024
-#define EVENT_TIMEOUT_SECONDS		10
-#define DAEMON_TIMEOUT_SECONDS		30
-
 
 static int expect_seqnum = 0;
-static struct hotplug_msg *head = NULL;
+static int lock_file = -1;
+static char *lock_filename = ".udevd_lock";
 
+LIST_HEAD(msg_list);
 
-static void sig_alarmhandler(int signum)
+static void sig_handler(int signum)
 {
 	dbg("caught signal %d", signum);
 	switch (signum) {
 	case SIGALRM:
 		dbg("event timeout reached");
 		break;
-
+	case SIGINT:
+	case SIGTERM:
+	case SIGKILL:
+		if (lock_file >= 0) {
+			close(lock_file);
+			unlink(lock_filename);
+		}
+		exit(20 + signum);
+		break;
 	default:
 		dbg("unhandled signal");
 	}
@@ -62,41 +72,32 @@
 
 static void dump_queue(void)
 {
-	struct hotplug_msg *p;
-	p = head;
+	struct hotplug_msg *msg;
 
-	dbg("next expected sequence is %d", expect_seqnum);
-	while(p != NULL) {
-		dbg("sequence %d in queue", p->seqnum);
-		p = p->next;
-	}
+	list_for_each_entry(msg, &msg_list, list)
+		dbg("sequence %d in queue", msg->seqnum);
 }
 
-static void dump_msg(struct hotplug_msg *pmsg)
+static void dump_msg(struct hotplug_msg *msg)
 {
 	dbg("sequence %d, '%s', '%s', '%s'",
-	    pmsg->seqnum, pmsg->action, pmsg->devpath, pmsg->subsystem);
+	    msg->seqnum, msg->action, msg->devpath, msg->subsystem);
 }
 
-static int dispatch_msg(struct hotplug_msg *pmsg)
+static int dispatch_msg(struct hotplug_msg *msg)
 {
 	pid_t pid;
-	char *argv[3];
-	extern char **environ;
 
-	dump_msg(pmsg);
+	dump_msg(msg);
 
-	setenv("ACTION", pmsg->action, 1);
-	setenv("DEVPATH", pmsg->devpath, 1);
-	argv[0] = DEFAULT_UDEV_EXEC;
-	argv[1] = pmsg->subsystem;
-	argv[2] = NULL;
+	setenv("ACTION", msg->action, 1);
+	setenv("DEVPATH", msg->devpath, 1);
 
 	pid = fork();
 	switch (pid) {
 	case 0:
 		/* child */
-		execve(argv[0], argv, environ);
+		execl(UDEV_EXEC, "udev", msg->subsystem, NULL);
 		dbg("exec of child failed");
 		exit(1);
 		break;
@@ -104,108 +105,119 @@
 		dbg("fork of child failed");
 		return -1;
 	default:
-		wait(0);
+		wait(NULL);
 	}
 	return 0;
 }
 
-static void set_timer(int seconds)
+static void set_timeout(int seconds)
 {
-	signal(SIGALRM, sig_alarmhandler);
 	alarm(seconds);
+	dbg("set timeout in %d seconds", seconds);
 }
 
 static void check_queue(void)
 {
-	struct hotplug_msg *p;
-	p = head;
-
-	dump_queue();
-	while(head != NULL && head->seqnum == expect_seqnum) {
-		dispatch_msg(head);
+	struct hotplug_msg *msg;
+	struct hotplug_msg *tmp_msg;
+	time_t msg_age;
+
+recheck:
+	/* dispatch events until one is missing */
+	list_for_each_entry_safe(msg, tmp_msg, &msg_list, list) {
+		if (msg->seqnum != expect_seqnum)
+			break;
+		dispatch_msg(msg);
 		expect_seqnum++;
-		p = head;
-		head = head->next;
-		free(p);
+		list_del_init(&msg->list);
+		free(msg);
+	}
+
+	/* recalculate timeout */
+	if (list_empty(&msg_list) == 0) {
+		msg_age = time(NULL) - msg->queue_time;
+		if (msg_age > EVENT_TIMEOUT_SECONDS-1) {
+			info("event %d, age %li seconds, skip event %d-%d",
+			     msg->seqnum, msg_age, expect_seqnum, msg->seqnum-1);
+			expect_seqnum = msg->seqnum;
+			goto recheck;
+		}
+		set_timeout(EVENT_TIMEOUT_SECONDS - msg_age);
+		return;
 	}
-	if (head != NULL)
-		set_timer(EVENT_TIMEOUT_SECONDS);
-	else
-		set_timer(DAEMON_TIMEOUT_SECONDS);
+
+	/* queue is empty */
+	set_timeout(UDEVD_TIMEOUT_SECONDS);
 }
 
-static void add_queue(struct hotplug_msg *pmsg)
+static int queue_msg(struct hotplug_msg *msg)
 {
-	struct hotplug_msg *pnewmsg;
-	struct hotplug_msg *p;
-	struct hotplug_msg *p1;
-
-	p = head;
-	p1 = NULL;
-	pnewmsg = malloc(sizeof(struct hotplug_msg));
-	*pnewmsg = *pmsg;
-	pnewmsg->next = NULL;
-	while(p != NULL && pmsg->seqnum > p->seqnum) {
-		p1 = p;
-		p = p->next;
-	}
-	pnewmsg->next = p;
-	if (p1 == NULL) {
-		head = pnewmsg;
-	} else {
-		p1->next = pnewmsg;
+	struct hotplug_msg *new_msg;
+	struct hotplug_msg *tmp_msg;
+
+	new_msg = malloc(sizeof(*new_msg));
+	if (new_msg == NULL) {
+		dbg("error malloc");
+		return -ENOMEM;
 	}
-	dump_queue();
-}
+	memcpy(new_msg, msg, sizeof(*new_msg));
 
-static int lock_file = -1;
-static char *lock_filename = ".udevd_lock";
+	/* store timestamp of queuing */
+	new_msg->queue_time = time(NULL);
+
+	/* sort message by sequence number into list*/
+	list_for_each_entry(tmp_msg, &msg_list, list)
+		if (tmp_msg->seqnum > new_msg->seqnum)
+			break;
+	list_add_tail(&new_msg->list, &tmp_msg->list);
+
+	return 0;
+}
 
-static int process_queue(void)
+static void work(void)
 {
+	struct hotplug_msg *msg;
 	int msgid;
 	key_t key;
-	struct hotplug_msg *pmsg;
 	char buf[BUFFER_SIZE];
 	int ret;
 
-	key = ftok(DEFAULT_UDEVD_EXEC, IPC_KEY_ID);
-	pmsg = (struct hotplug_msg *) buf;
+	key = ftok(UDEVD_EXEC, IPC_KEY_ID);
+	msg = (struct hotplug_msg *) buf;
 	msgid = msgget(key, IPC_CREAT);
 	if (msgid == -1) {
 		dbg("open message queue error");
-		return -1;
+		exit(1);
 	}
 	while (1) {
 		ret = msgrcv(msgid, (struct msgbuf *) buf, BUFFER_SIZE-4, HOTPLUGMSGTYPE, 0);
 		if (ret != -1) {
-			dbg("current sequence %d, expected sequence %d", pmsg->seqnum, expect_seqnum);
-
-			/* init expected sequence with value from first call */
+			/* init the expected sequence with value from first call */
 			if (expect_seqnum == 0) {
-				expect_seqnum = pmsg->seqnum;
+				expect_seqnum = msg->seqnum;
 				dbg("init next expected sequence number to %d", expect_seqnum);
 			}
-
-			if (pmsg->seqnum > expect_seqnum) {
-				add_queue(pmsg);
-				set_timer(EVENT_TIMEOUT_SECONDS);
-			} else {
-				if (pmsg->seqnum == expect_seqnum) {
-					dispatch_msg(pmsg);
-					expect_seqnum++;
-					check_queue();
-				} else {
-					dbg("timeout event for unexpected sequence number %d", pmsg->seqnum);
-				}
+			dbg("current sequence %d, expected sequence %d", msg->seqnum, expect_seqnum);
+			if (msg->seqnum == expect_seqnum) {
+				/* execute expected event */
+				dispatch_msg(msg);
+				expect_seqnum++;
+				check_queue();
+				dump_queue();
+				continue;
 			}
+			if (msg->seqnum > expect_seqnum) {
+				/* something missing, queue event*/
+				queue_msg(msg);
+				check_queue();
+				dump_queue();
+				continue;
+			}
+			dbg("too late for event with sequence %d, even skipped ", msg->seqnum);
 		} else {
 			if (errno == EINTR) {
-				if (head != NULL) {
-					/* event timeout, skip all missing, proceed with next queued event */
-					info("timeout reached, skip events %d - %d", expect_seqnum, head->seqnum-1);
-					expect_seqnum = head->seqnum;
-				} else {
+				/* timeout */
+				if (list_empty(&msg_list)) {
 					info("we have nothing to do, so daemon exits...");
 					if (lock_file >= 0) {
 						close(lock_file);
@@ -214,31 +226,12 @@
 					exit(0);
 				}
 				check_queue();
-			} else {
-				dbg("ipc message receive error '%s'", strerror(errno));
+				dump_queue();
+				continue;
 			}
+			dbg("ipc message receive error '%s'", strerror(errno));
 		}
 	}
-	return 0;
-}
-
-static void sig_handler(int signum)
-{
-	dbg("caught signal %d", signum);
-	switch (signum) {
-		case SIGINT:
-		case SIGTERM:
-		case SIGKILL:
-			if (lock_file >= 0) {
-				close(lock_file);
-				unlink(lock_filename);
-			}
-			exit(20 + signum);
-			break;
-
-		default:
-			dbg("unhandled signal");
-	}
 }
 
 static int one_and_only(void)
@@ -249,18 +242,18 @@
 
 	/* see if we can open */
 	if (lock_file < 0)
-		return -EINVAL;
+		return -1;
 	
 	/* see if we can lock */
 	if (lockf(lock_file, F_TLOCK, 0) < 0) {
 		close(lock_file);
 		unlink(lock_filename);
-		return -EINVAL;
+		return -1;
 	}
 
 	snprintf(string, sizeof(string), "%d\n", getpid());
 	write(lock_file, string, strlen(string));
-	
+
 	return 0;
 }
 
@@ -274,11 +267,11 @@
 	signal(SIGINT, sig_handler);
 	signal(SIGTERM, sig_handler);
 	signal(SIGKILL, sig_handler);
+	signal(SIGALRM, sig_handler);
 
 	/* we exit if we have nothing to do, next event will start us again */
-	set_timer(DAEMON_TIMEOUT_SECONDS);
+	set_timeout(UDEVD_TIMEOUT_SECONDS);
 
-	/* main loop */
-	process_queue();
-	return 0;
+	work();
+	exit(0);
 }
diff -Nru a/udevd.h b/udevd.h
--- a/udevd.h	Sun Jan 25 20:57:21 2004
+++ b/udevd.h	Sun Jan 25 20:57:21 2004
@@ -21,17 +21,22 @@
  *
  */
 
-#define DEFAULT_UDEV_EXEC	"./udev"
-#define DEFAULT_UDEVD_EXEC	"./udevd"
+#include "list.h"
 
-#define IPC_KEY_ID		0
-#define HOTPLUGMSGTYPE		44
+#define UDEV_EXEC			"./udev"
+#define UDEVD_EXEC			"./udevd"
+#define UDEVD_TIMEOUT_SECONDS		60
+#define EVENT_TIMEOUT_SECONDS		5
+
+#define IPC_KEY_ID			0
+#define HOTPLUGMSGTYPE			44
 
 
 struct hotplug_msg {
 	long mtype;
-	struct hotplug_msg *next;
+	struct list_head list;
 	int seqnum;
+	time_t queue_time;
 	char action[8];
 	char devpath[128];
 	char subsystem[16];
diff -Nru a/udevsend.c b/udevsend.c
--- a/udevsend.c	Sun Jan 25 20:57:21 2004
+++ b/udevsend.c	Sun Jan 25 20:57:21 2004
@@ -64,6 +64,7 @@
 static int build_hotplugmsg(struct hotplug_msg *msg, char *action,
 			    char *devpath, char *subsystem, int seqnum)
 {
+	memset(msg, 0x00, sizeof(msg));
 	msg->mtype = HOTPLUGMSGTYPE;
 	msg->seqnum = seqnum;
 	strncpy(msg->action, action, 8);
@@ -85,7 +86,8 @@
 		switch (child_pid) {
 		case 0:
 			/* daemon */
-			execl(DEFAULT_UDEVD_EXEC, NULL);
+			setsid();
+			execl(UDEVD_EXEC, "udevd", NULL);
 			dbg("exec of daemon failed");
 			exit(1);
 		case -1:
@@ -99,7 +101,7 @@
 		dbg("fork of helper failed");
 		return -1;
 	default:
-		wait(0);
+		wait(NULL);
 	}
 	return 0;
 }
@@ -147,7 +149,7 @@
 	seq = atoi(seqnum);
 
 	/* create ipc message queue or get id of our existing one */
-	key = ftok(DEFAULT_UDEVD_EXEC, IPC_KEY_ID);
+	key = ftok(UDEVD_EXEC, IPC_KEY_ID);
 	size =  build_hotplugmsg(&message, action, devpath, subsystem, seq);
 	msgid = msgget(key, IPC_CREAT);
 	if (msgid == -1) {
@@ -165,7 +167,7 @@
 	/* get state of ipc queue */
 	tspec.tv_sec = 0;
 	tspec.tv_nsec = 10000000;  /* 10 millisec */
-	loop = 20;
+	loop = 30;
 	while (loop--) {
 		retval = msgctl(msgid, IPC_STAT, &msg_queue);
 		if (retval == -1) {

^ permalink raw reply	[flat|nested] 32+ messages in thread