Cleanup alias module handler register/unregister.

- Remove locking, since all module(9) events are running under &Giant.
- Use TAILQ for protocol handlers and fix a bug which led to
  infinite cycle. Bug found in VirtualBox [1]
- Simplify code everywhere.
- Fix documentation.

[1]  https://www.virtualbox.org/pipermail/vbox-dev/2013-November/011936.html

PR:		183792 [1]
Submitted by:	Valery Ushakov <uwe NetBSD.org> [1]
Sponsored by:	Nginx, Inc.

4 files changed, 63 insertions, 186 deletions

diff --git a/sys/netinet/libalias/alias_db.c b/sys/netinet/libalias/alias_db.c
index 7df55d0..cceccd5 100644
--- a/sys/netinet/libalias/alias_db.c
+++ b/sys/netinet/libalias/alias_db.c
@@ -349,24 +349,16 @@ MODULE_VERSION(libalias, 1);
 static int
 alias_mod_handler(module_t mod, int type, void *data)
 {
-	int error;
 
 	switch (type) {
-	case MOD_LOAD:
-		error = 0;
-		handler_chain_init();
-		break;
 	case MOD_QUIESCE:
 	case MOD_UNLOAD:
-	        handler_chain_destroy();
 	        finishoff();
-		error = 0;
-		break;
+	case MOD_LOAD:
+		return (0);
 	default:
-		error = EINVAL;
+		return (EINVAL);
 	}
-
-	return (error);
 }
 
 static moduledata_t alias_mod = {
diff --git a/sys/netinet/libalias/alias_mod.c b/sys/netinet/libalias/alias_mod.c
index a061431..6ac4424 100644
--- a/sys/netinet/libalias/alias_mod.c
+++ b/sys/netinet/libalias/alias_mod.c
@@ -52,201 +52,82 @@ __FBSDID("$FreeBSD$");
 #endif
 
 /* Protocol and userland module handlers chains. */
-LIST_HEAD(handler_chain, proto_handler) handler_chain = LIST_HEAD_INITIALIZER(handler_chain);
-#ifdef _KERNEL
-struct rwlock   handler_rw;
-#endif
-SLIST_HEAD(dll_chain, dll) dll_chain = SLIST_HEAD_INITIALIZER(dll_chain);
-
-#ifdef _KERNEL
-
-#define	LIBALIAS_RWLOCK_INIT() \
-	rw_init(&handler_rw, "Libalias_modules_rwlock")
-#define	LIBALIAS_RWLOCK_DESTROY()	rw_destroy(&handler_rw)
-#define	LIBALIAS_WLOCK_ASSERT() \
-	rw_assert(&handler_rw, RA_WLOCKED)
-
-static __inline void
-LIBALIAS_RLOCK(void)
-{
-	rw_rlock(&handler_rw);
-}
-
-static __inline void
-LIBALIAS_RUNLOCK(void)
-{
-	rw_runlock(&handler_rw);
-}
-
-static __inline void
-LIBALIAS_WLOCK(void)
-{
-	rw_wlock(&handler_rw);
-}
-
-static __inline void
-LIBALIAS_WUNLOCK(void)
-{
-	rw_wunlock(&handler_rw);
-}
-
-static void
-_handler_chain_init(void)
-{
-
-	if (!rw_initialized(&handler_rw))
-		LIBALIAS_RWLOCK_INIT();
-}
-
-static void
-_handler_chain_destroy(void)
-{
-
-	if (rw_initialized(&handler_rw))
-		LIBALIAS_RWLOCK_DESTROY();
-}
-
-#else
-#define	LIBALIAS_RWLOCK_INIT() ;
-#define	LIBALIAS_RWLOCK_DESTROY()	;
-#define	LIBALIAS_WLOCK_ASSERT()	;
-#define	LIBALIAS_RLOCK() ;
-#define	LIBALIAS_RUNLOCK() ;
-#define	LIBALIAS_WLOCK() ;
-#define	LIBALIAS_WUNLOCK() ;
-#define _handler_chain_init() ;
-#define _handler_chain_destroy() ;
-#endif
-
-void
-handler_chain_init(void)
-{
-	_handler_chain_init();
-}
-
-void
-handler_chain_destroy(void)
-{
-	_handler_chain_destroy();
-}
+static TAILQ_HEAD(handler_chain, proto_handler) handler_chain =
+    TAILQ_HEAD_INITIALIZER(handler_chain);
 
 static int
-_attach_handler(struct proto_handler *p)
+attach_handler(struct proto_handler *p)
 {
 	struct proto_handler *b;
 
-	LIBALIAS_WLOCK_ASSERT();
-	b = NULL;
-	LIST_FOREACH(b, &handler_chain, entries) {
+	TAILQ_FOREACH(b, &handler_chain, link) {
 		if ((b->pri == p->pri) &&
 		    (b->dir == p->dir) &&
 		    (b->proto == p->proto))
-			return (EEXIST); /* Priority conflict. */
+			return (EEXIST);
 		if (b->pri > p->pri) {
-			LIST_INSERT_BEFORE(b, p, entries);
+			TAILQ_INSERT_BEFORE(b, p, link);
 			return (0);
 		}
 	}
-	/* End of list or found right position, inserts here. */
-	if (b)
-		LIST_INSERT_AFTER(b, p, entries);
-	else
-		LIST_INSERT_HEAD(&handler_chain, p, entries);
-	return (0);
-}
 
-static int
-_detach_handler(struct proto_handler *p)
-{
-	struct proto_handler *b, *b_tmp;
+	TAILQ_INSERT_TAIL(&handler_chain, p, link);
 
-	LIBALIAS_WLOCK_ASSERT();
-	LIST_FOREACH_SAFE(b, &handler_chain, entries, b_tmp) {
-		if (b == p) {
-			LIST_REMOVE(b, entries);
-			return (0);
-		}
-	}
-	return (ENOENT); /* Handler not found. */
+	return (0);
 }
 
 int
-LibAliasAttachHandlers(struct proto_handler *_p)
+LibAliasAttachHandlers(struct proto_handler *p)
 {
-	int i, error;
+	int error;
 
-	LIBALIAS_WLOCK();
-	error = -1;
-	for (i = 0; 1; i++) {
-		if (*((int *)&_p[i]) == EOH)
-			break;
-		error = _attach_handler(&_p[i]);
-		if (error != 0)
-			break;
+	while (p->dir != NODIR) {
+		error = attach_handler(p);
+		if (error)
+			return (error);
+		p++;
 	}
-	LIBALIAS_WUNLOCK();
-	return (error);
+
+	return (0);
 }
 
+/* XXXGL: should be void, but no good reason to break ABI */
 int
-LibAliasDetachHandlers(struct proto_handler *_p)
+LibAliasDetachHandlers(struct proto_handler *p)
 {
-	int i, error;
 
-	LIBALIAS_WLOCK();
-	error = -1;
-	for (i = 0; 1; i++) {
-		if (*((int *)&_p[i]) == EOH)
-			break;
-		error = _detach_handler(&_p[i]);
-		if (error != 0)
-			break;
+	while (p->dir != NODIR) {
+		TAILQ_REMOVE(&handler_chain, p, link);
+		p++;
 	}
-	LIBALIAS_WUNLOCK();
-	return (error);
-}
-
-int
-detach_handler(struct proto_handler *_p)
-{
-	int error;
 
-	LIBALIAS_WLOCK();
-	error = -1;
-	error = _detach_handler(_p);
-	LIBALIAS_WUNLOCK();
-	return (error);
+	return (0);
 }
 
 int
-find_handler(int8_t dir, int8_t proto, struct libalias *la, __unused struct ip *pip,
+find_handler(int8_t dir, int8_t proto, struct libalias *la, struct ip *ip,
     struct alias_data *ad)
 {
 	struct proto_handler *p;
-	int error;
 
-	LIBALIAS_RLOCK();
-	error = ENOENT;
-	LIST_FOREACH(p, &handler_chain, entries) {
-		if ((p->dir & dir) && (p->proto & proto))
-			if (p->fingerprint(la, ad) == 0) {
-				error = p->protohandler(la, pip, ad);
-				break;
-			}
-	}
-	LIBALIAS_RUNLOCK();
-	return (error);
+	TAILQ_FOREACH(p, &handler_chain, link)
+		if ((p->dir & dir) && (p->proto & proto) &&
+		    p->fingerprint(la, ad) == 0)
+			return (p->protohandler(la, ip, ad));
+
+	return (ENOENT);
 }
 
 struct proto_handler *
 first_handler(void)
 {
 
-	return (LIST_FIRST(&handler_chain));
+	return (TAILQ_FIRST(&handler_chain));
 }
 
 #ifndef _KERNEL
 /* Dll manipulation code - this code is not thread safe... */
+SLIST_HEAD(dll_chain, dll) dll_chain = SLIST_HEAD_INITIALIZER(dll_chain);
 int
 attach_dll(struct dll *p)
 {
diff --git a/sys/netinet/libalias/alias_mod.h b/sys/netinet/libalias/alias_mod.h
index c24c0bc..12cb1be 100644
--- a/sys/netinet/libalias/alias_mod.h
+++ b/sys/netinet/libalias/alias_mod.h
@@ -45,14 +45,15 @@ MALLOC_DECLARE(M_ALIAS);
 #endif
 #endif
 
-/* Packet flow direction. */
-#define IN	1
-#define OUT	2
+/* Packet flow direction flags. */
+#define IN	0x0001
+#define OUT	0x0002
+#define	NODIR	0x4000
 
-/* Working protocol. */
-#define IP	1
-#define TCP	2
-#define UDP	4
+/* Working protocol flags. */
+#define IP	0x01
+#define TCP	0x02
+#define UDP	0x04
 
 /*
  * Data passed to protocol handler module, it must be filled
@@ -81,18 +82,15 @@ struct proto_handler {
 	/* Aliasing * function. */
 	int (*protohandler)(struct libalias *, struct ip *,
 	    struct alias_data *);
-	LIST_ENTRY(proto_handler) entries;
-}
-;
+	TAILQ_ENTRY(proto_handler) link;
+};
+
 /* End of handlers. */
-#define EOH     -1
+#define EOH	.dir = NODIR
 
 /* Functions used with protocol handlers. */
-void handler_chain_init(void);
-void handler_chain_destroy(void);
 int LibAliasAttachHandlers(struct proto_handler *);
 int LibAliasDetachHandlers(struct proto_handler *);
-int detach_handler(struct proto_handler *);
 int find_handler(int8_t, int8_t, struct libalias *, struct ip *,
     struct alias_data *);
 struct proto_handler *first_handler(void);
diff --git a/sys/netinet/libalias/libalias.3 b/sys/netinet/libalias/libalias.3
index 4d02cc4..45e7fa4 100644
--- a/sys/netinet/libalias/libalias.3
+++ b/sys/netinet/libalias/libalias.3
@@ -25,7 +25,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd July 4, 2011
+.Dd December 25, 2013
 .Dt LIBALIAS 3
 .Os
 .Sh NAME
@@ -1103,7 +1103,7 @@ struct proto_handler {
 		 struct ip *pip, struct alias_data *ah);
 	int (*protohandler)(struct libalias *la,
 		 struct ip *pip, struct alias_data *ah);
-	LIST_ENTRY(proto_handler) entries;
+	TAILQ_ENTRY(proto_handler) link;
 };
 .Ed
 .Pp
@@ -1262,8 +1262,16 @@ Here we analyse some parts of that module.
 From
 .Pa alias_dummy.c :
 .Bd -literal
-struct proto_handler handlers [] = {{666, IN|OUT, UDP|TCP,
-				    &fingerprint, &protohandler}};
+struct proto_handler handlers[] = {
+    {
+	.pri = 666,
+	.dir = IN|OUT,
+	.proto = UDP|TCP,
+	.fingerprint = fingerprint,
+	.protohandler= protohandler,
+    },
+    { EOH }
+};
 .Ed
 .Pp
 The variable
@@ -1299,12 +1307,10 @@ mod_handler(module_t mod, int type, void *data)
 
 	switch (type) {
 	case MOD_LOAD:
-		error = 0;
-		attach_handlers(handlers);
+		error = LibAliasAttachHandlers(handlers);
 		break;
 	case MOD_UNLOAD:
-		error = 0;
-		detach_handlers(handlers;
+		error = LibAliasDetachHandlers(handlers);
 		break;
 	default:
 		error = EINVAL;
@@ -1315,9 +1321,9 @@ mod_handler(module_t mod, int type, void *data)
 When running as KLD,
 .Fn mod_handler
 registers/deregisters the module using
-.Fn attach_handlers
+.Fn LibAliasAttachHandlers
 and
-.Fn detach_handlers ,
+.Fn LibAliasDetachHandlers ,
 respectively.
 .Pp
 Every module must contain at least 2 functions: one fingerprint