[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Cluster-devel] Groupd uevent clean up



The following patch is designed to clean up a number of items relating to
gfs_controld's handling of uevent notifications. When a uevent is received
is consists of a number of strings whose total length is bounded by the
buffer size and returned by the recv call.

Originally only the initial string was being used to select the various
actions. That was wrong for various reasons, but mostly because its much
easier to simply look for the individual fields, broken out from the
message. The other reason that this is wrong, and also the reason that
I started looking into this area initially is that the initial event
string provides a "physical" view of the event. By using the SUBSYSTEM=
field, we get the same information, but it can also be overridden by
the kernel using the kset_uevent_ops which will become important when
we merge the lock_dlm module into GFS2.

The other problem (unsolved here) is that we have to parse the fs name
from the DEVPATH= variable. Its trivial to add a feature to the kernel
to provide the LOCKTABLE as well, so I plan to do that in the future
so we don't have to jump through hoops to get that information.

At the same time I've marked a bunch of variables const. Mainly
because they started to throw up warnings after I'd added the
new event parsing code.

I've tested the parsing code on its own, but since I don't have the
right packages installed here, I've not had a chance to build &
test a complete new groupd yet.

Also, I've fixed up some broken error handling in process_uevent
at the same time.

Signed-off-by: Steven Whitehouse <swhiteho redhat com>

diff --git a/group/gfs_controld/cpg-new.c b/group/gfs_controld/cpg-new.c
index 020b89b..00ffc86 100644
--- a/group/gfs_controld/cpg-new.c
+++ b/group/gfs_controld/cpg-new.c
@@ -2078,7 +2078,7 @@ static void apply_changes(struct mountgroup *mg)
    and then process the uevent/ipc upon receiving the message for it, so
    that it can be processed in the same order by all nodes. */
 
-void process_recovery_uevent(char *table)
+void process_recovery_uevent(const char *table)
 {
 	struct mountgroup *mg;
 	struct journal *j;
@@ -2678,7 +2678,7 @@ static void leave_mountgroup(struct mountgroup *mg, int mnterr)
 		log_error("cpg_leave error %d", error);
 }
 
-void do_leave(char *table, int mnterr)
+void do_leave(const char *table, int mnterr)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
diff --git a/group/gfs_controld/cpg-old.c b/group/gfs_controld/cpg-old.c
index 192a403..d59d6ca 100644
--- a/group/gfs_controld/cpg-old.c
+++ b/group/gfs_controld/cpg-old.c
@@ -1604,7 +1604,7 @@ static int need_kernel_recovery_done(struct mountgroup *mg)
    remain blocked until an rw node mounts, and the next mounter must
    be rw. */
 
-int process_recovery_uevent_old(char *table)
+int process_recovery_uevent_old(const char *table)
 {
 	struct mountgroup *mg;
 	struct mg_member *memb;
@@ -1724,7 +1724,7 @@ static void leave_mountgroup(struct mountgroup *mg, int mnterr)
 	group_leave(gh, mg->name);
 }
 
-void do_leave_old(char *table, int mnterr)
+void do_leave_old(const char *table, int mnterr)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
diff --git a/group/gfs_controld/gfs_daemon.h b/group/gfs_controld/gfs_daemon.h
index 3beeb0e..ecb45d2 100644
--- a/group/gfs_controld/gfs_daemon.h
+++ b/group/gfs_controld/gfs_daemon.h
@@ -225,10 +225,10 @@ void process_cpg(int ci);
 int setup_dlmcontrol(void);
 void process_dlmcontrol(int ci);
 int set_protocol(void);
-void process_recovery_uevent(char *table);
+void process_recovery_uevent(const char *table);
 void process_mountgroups(void);
 int gfs_join_mountgroup(struct mountgroup *mg);
-void do_leave(char *table, int mnterr);
+void do_leave(const char *table, int mnterr);
 void gfs_mount_done(struct mountgroup *mg);
 void send_remount(struct mountgroup *mg, struct gfsc_mount_args *ma);
 void send_withdraw(struct mountgroup *mg);
@@ -245,12 +245,12 @@ void close_cpg_old(void);
 void process_cpg_old(int ci);
 
 int gfs_join_mountgroup_old(struct mountgroup *mg, struct gfsc_mount_args *ma);
-void do_leave_old(char *table, int mnterr);
+void do_leave_old(const char *table, int mnterr);
 int send_group_message_old(struct mountgroup *mg, int len, char *buf);
 void save_message_old(struct mountgroup *mg, char *buf, int len, int from,
 	int type);
 void send_withdraw_old(struct mountgroup *mg);
-int process_recovery_uevent_old(char *table);
+int process_recovery_uevent_old(const char *table);
 void ping_kernel_mount_old(char *table);
 void send_remount_old(struct mountgroup *mg, struct gfsc_mount_args *ma);
 void send_mount_status_old(struct mountgroup *mg);
@@ -282,7 +282,7 @@ int client_fd(int ci);
 void client_ignore(int ci, int fd);
 void client_back(int ci, int fd);
 struct mountgroup *create_mg(char *name);
-struct mountgroup *find_mg(char *name);
+struct mountgroup *find_mg(const char *name);
 struct mountgroup *find_mg_id(uint32_t id);
 void client_reply_remount(struct mountgroup *mg, int ci, int result);
 void client_reply_join(int ci, struct gfsc_mount_args *ma, int result);
diff --git a/group/gfs_controld/main.c b/group/gfs_controld/main.c
index 09fb105..ee82612 100644
--- a/group/gfs_controld/main.c
+++ b/group/gfs_controld/main.c
@@ -22,7 +22,7 @@ struct client {
 	struct mountgroup *mg;
 };
 
-static void do_withdraw(char *table);
+static void do_withdraw(const char *table);
 
 int do_read(int fd, void *buf, size_t count)
 {
@@ -175,7 +175,7 @@ struct mountgroup *create_mg(char *name)
 	return mg;
 }
 
-struct mountgroup *find_mg(char *name)
+struct mountgroup *find_mg(const char *name)
 {
 	struct mountgroup *mg;
 
@@ -198,39 +198,7 @@ struct mountgroup *find_mg_id(uint32_t id)
 	return NULL;
 }
 
-#define MAXARGS 8
-
-static char *get_args(char *buf, int *argc, char **argv, char sep, int want)
-{
-	char *p = buf, *rp = NULL;
-	int i;
-
-	argv[0] = p;
-
-	for (i = 1; i < MAXARGS; i++) {
-		p = strchr(buf, sep);
-		if (!p)
-			break;
-		*p = '\0';
-
-		if (want == i) {
-			rp = p + 1;
-			break;
-		}
-
-		argv[i] = p + 1;
-		buf = p + 1;
-	}
-	*argc = i;
-
-	/* we ended by hitting \0, return the point following that */
-	if (!rp)
-		rp = strchr(buf, '\0') + 1;
-
-	return rp;
-}
-
-static void ping_kernel_mount(char *table)
+static void ping_kernel_mount(const char *table)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;
@@ -245,89 +213,92 @@ static void ping_kernel_mount(char *table)
 	log_group(mg, "ping_kernel_mount %d", rv);
 }
 
-static void process_uevent(int ci)
+/* FIXME: We should have a LOCKTABLE env var rather than trying to 
+ * parse it from DEVPATH
+ */
+
+static void decode_buf(char *buf, int len, const char **action,
+		       const char **subsystem, const char **fsname)
 {
-	char buf[MAXLINE];
-	char *argv[MAXARGS], *act, *sys;
-	int rv, argc = 0;
-	int lock_module = 0;
+	char *ptr;
+
+	*fsname = *subsystem = *action = NULL;
+	while (len > 8) {
+		ptr = buf;
+		buf += strlen(ptr);
+		len -= strlen(ptr);
+		buf++; len--;
+		if (strlen(ptr) < 7)
+			continue;
+		if (strncmp(ptr, "ACTION=", 7) == 0)
+			*action = ptr + 7;
+		if (strncmp(ptr, "SUBSYSTEM=", 10) == 0)
+			*subsystem = ptr + 10;
+		if (strncmp(ptr, "DEVPATH=/fs/gfs", 15) == 0) {
+			ptr += 15;
+			if (*ptr == '2')
+				ptr++;
+			if (*ptr != '/')
+				continue;
+			*fsname = ++ptr;
+			while (*ptr && *ptr != '/')
+				ptr++;
+			*ptr = 0;
+		}
+	}
+}
 
-	memset(buf, 0, sizeof(buf));
-	memset(argv, 0, sizeof(char *) * MAXARGS);
+static void process_uevent(int ci)
+{
+	char buf[MAXLINE + 1];
+	const char *action, *subsystem, *fsname;
+	int rv;
 
  retry_recv:
-	rv = recv(client[ci].fd, &buf, sizeof(buf), 0);
-	if (rv == -1 && rv == EINTR)
-		goto retry_recv;
-	if (rv == -1 && rv == EAGAIN)
-		return;
+	rv = recv(client[ci].fd, &buf, MAXLINE, 0);
 	if (rv < 0) {
-		log_error("uevent recv error %d errno %d", rv, errno);
+		if (errno == EINTR)
+			goto retry_recv;
+		if (errno != EAGAIN)
+			log_error("uevent recv error %d errno %d", rv, errno);
 		return;
 	}
-
-	/* first we get the uevent for removing lock module kobject:
-	     "remove@/fs/gfs/bull:x/lock_module"
-	   second is the uevent for removing gfs kobject:
-	     "remove@/fs/gfs/bull:x"
-	*/
-
-	if (!strstr(buf, "gfs"))
+	buf[rv] = 0;
+	decode_buf(buf, rv, &action, &subsystem, &fsname);
+	if (!fsname || !action || !subsystem)
 		return;
 
-	/* if an fs is named "gfs", it results in dlm uevents
-	   like "remove@/kernel/dlm/gfs" */
-
-	if (strstr(buf, "kernel/dlm"))
-		return;
-
-	log_debug("uevent: %s", buf);
-
-	if (strstr(buf, "lock_module"))
-		lock_module = 1;
-
-	get_args(buf, &argc, argv, '/', 4);
-	if (argc != 4)
-		log_error("uevent message has %d args", argc);
-	act = argv[0];
-	sys = argv[2];
-
-	log_debug("kernel: %s %s", act, argv[3]);
-
-	if (!strcmp(act, "remove@")) {
+/* FIXME: All of the below functions call find_mg as pretty much
+ * the first thing that they do. This ought to be done here and
+ * the found mg (if any) passed to the routines.
+ */
+	if (strcmp(action, "remove") == 0) {
 		/* We want to trigger the leave at the very end of the kernel's
 		   unmount process, i.e. at the end of put_super(), so we do the
 		   leave when the second uevent (from the gfs kobj) arrives. */
 
-		if (lock_module)
+		if (strcmp(subsystem, "lock_dlm") == 0)
 			return;
-
 		if (group_mode == GROUP_LIBGROUP)
-			do_leave_old(argv[3], 0);
+			do_leave_old(fsname, 0);
 		else
-			do_leave(argv[3], 0);
-
-	} else if (!strcmp(act, "change@")) {
-		if (!lock_module)
-			return;
+			do_leave(fsname, 0);
 
+	}
+	if (strcmp(subsystem, "lock_dlm") != 0)
+		return;
+	if (strcmp(action, "change") == 0) {
 		if (group_mode == GROUP_LIBGROUP)
-			process_recovery_uevent_old(argv[3]);
+			process_recovery_uevent_old(fsname);
 		else
-			process_recovery_uevent(argv[3]);
-
-	} else if (!strcmp(act, "offline@")) {
-		if (!lock_module)
-			return;
-
-		do_withdraw(argv[3]);
-
-	} else {
-		if (!lock_module)
-			return;
-
-		ping_kernel_mount(argv[3]);
+			process_recovery_uevent(fsname);
+		return;
+	}
+	if (strcmp(action, "offline") == 0) {
+		do_withdraw(fsname);
+		return;
 	}
+	ping_kernel_mount(fsname);
 }
 
 static int setup_uevent(void)
@@ -737,7 +708,7 @@ static void do_join(int ci, struct gfsc_mount_args *ma)
    and when it's been removed from the group, it tells the locally withdrawing
    gfs to clear out locks. */
 
-static void do_withdraw(char *table)
+static void do_withdraw(const char *table)
 {
 	struct mountgroup *mg;
 	char *name = strstr(table, ":") + 1;



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]