[master] 7b21e7e Start introducing "struct suckaddr" which will become our new VCL_IP type.

Poul-Henning Kamp phk at varnish-cache.org
Sun Oct 27 21:46:54 CET 2013


commit 7b21e7e3a735d472162f7f9e05e89a44bf9855ef
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date:   Sun Oct 27 20:46:25 2013 +0000

    Start introducing "struct suckaddr" which will become our new VCL_IP type.

diff --git a/bin/varnishd/flint.lnt b/bin/varnishd/flint.lnt
index e203cb8..bfa3ccb 100644
--- a/bin/varnishd/flint.lnt
+++ b/bin/varnishd/flint.lnt
@@ -1,4 +1,5 @@
 -d__flexelint_v9__=1
++fan
 
 -printf(3, VSL)
 -printf(2, http_PrintfHeader)
diff --git a/include/vrt.h b/include/vrt.h
index e203168..18b0fba 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -42,6 +42,7 @@ struct cli;
 struct director;
 struct VCL_conf;
 struct sockaddr_storage;
+struct suckaddr;
 
 /***********************************************************************
  * This is the central definition of the mapping from VCL types to
@@ -225,6 +226,9 @@ void VRT_synth_page(const struct vrt_ctx *, unsigned flags, const char *, ...);
 void VRT_init_dir(struct cli *, struct director **, int idx, const void *priv);
 void VRT_fini_dir(struct cli *, struct director *);
 
+/* Suckaddr related */
+int VRT_VSA_GetPtr(const struct suckaddr *sua, const unsigned char ** dst);
+
 /* VMOD/Modules related */
 int VRT_Vmod_Init(void **hdl, void *ptr, int len, const char *nm,
     const char *path, struct cli *cli);
diff --git a/include/vsa.h b/include/vsa.h
index 3ac8eb2..c1e5509 100644
--- a/include/vsa.h
+++ b/include/vsa.h
@@ -30,6 +30,9 @@
 #ifndef VSA_H_INCLUDED
 #define VSA_H_INCLUDED
 
+struct suckaddr;
+extern const int vsa_suckaddr_len;
+
 int VSA_Sane(const void *);
 socklen_t VSA_Len(const void *);
 unsigned VSA_Port(const void *);
diff --git a/lib/libvarnish/vsa.c b/lib/libvarnish/vsa.c
index 1f2a92f..000678d 100644
--- a/lib/libvarnish/vsa.c
+++ b/lib/libvarnish/vsa.c
@@ -40,6 +40,162 @@
 
 #include "vas.h"
 #include "vsa.h"
+#include "vrt.h"
+
+/*
+ * Struct sockaddr{|_in|_in6|_storage} is absolutely the worst data
+ * structure I have ever seen gold-plated in international standards.
+ *
+ * Network addresses have multiple different forms, many fewer today
+ * than in last century, but imagine that in addition to IPv4 and IPv6
+ * we had 40 other protocols.  Actually, you don't need to imagine that
+ * just count the AF_* macros in /usr/include/sys/socket.h.
+ *
+ * So what do we pass the kernel API for an address to bind(2), connect(2) &
+ * listen(2) etc. etc ?
+ *
+ * We could define a struct which is big enough to hold any and all
+ * of these addresses.  That would make it a fixed size argument.
+ * obviously the struct would have to be something like:
+ *	struct bla {
+ *		int family;
+ *		char address[MAX_ADDR_LEN];
+ *	}
+ * and MAX_ADDR_LEN would have to be quite large, 128 byte or so.
+ *
+ * Back in last century that was TOTALLY unacceptable waste of space.
+ *
+ * The way which was chosen instead, was to make a "generic" address,
+ * and have per protocol "specific" addresses, and pass the length
+ * argument explicitly to the KPI functions.
+ *
+ * The generic address was called "struct sockaddr", and the specific
+ * were called "struct sockaddr_${whatever}".  All of these must have
+ * a "family" field as first element, so the kernel can figure out
+ * which protocol it is.
+ *
+ * The generic struct sockaddr was made big enough for all protocols
+ * supported in the kernel, so it would have different sizes depending
+ * on your machine and kernel configuration.
+ *
+ * However, that allowed you to write protocol-agnostic programs, by
+ * using "struct sockaddr" throughout, and relying on libray APIs for
+ * things like name to address (and vice versa) resolution, and since
+ * nobody were in the business of shipping random UNIX binaries around
+ * the lack of binary portability didn't matter.
+ *
+ * Along the way the BSD people figured out that it was a bother
+ * to carry the length argument separately, and added that to the
+ * format of sockaddr, but other groups found this unclean, as
+ * the length was already an explicit paramter.
+ *
+ * The net result of this is that your "portable" code, must take
+ * care to handle the "sa_len" member on kernels which have it,
+ * while still tracking the separate length argument for all other
+ * kernels.
+ *
+ * Needless to say, there were no neat #define to tell you which
+ * was which, so each programmer found a different heuristic to
+ * decide, often not understanding it fully, which caused the kind
+ * of portability issues which lead to the autocrap tools.
+ *
+ * Then all the other protocols died, we were left with IP and
+ * life were good, the dot-com madness multiplied the IT-business
+ * by a factor 1000, by making any high-school student who had
+ * programmed PERL for 6 weeks a "senior web-programmer".
+ *
+ * Next IPv6 happened, in a rush even, (no seriously, I'm not kidding!),
+ * and since IPv6 addresses were HUGE, like 16 bytes HUGE, the generic
+ * struct sockaddr was not increased in size.
+ *
+ * At least "not yet", because it would break all the shitty code written
+ * by the dot-com generation.
+ *
+ * Nobody used IPv6 anyway so that didn't matter that much.
+ *
+ * Then people actually started using IPv6 and its struct sockaddr_in6,
+ * and realized that all the code which used "struct sockaddr" to allocate
+ * space at compile time were broken.
+ *
+ * Some people took to using sockaddr_in6, since that was known to
+ * be big enough for both IPv4 and IPv6, but "purist" found that
+ * ugly and "prone to future trouble".
+ *
+ * So instead they came up with a "clean solution":  The added
+ * "struct sockaddr_storage" which is defined to be "Large enough
+ * to accommodate all supported protocol-specific address structures".
+ *
+ * Since we cannot possibly know what zany protocols will exist in
+ * the future, and since some people think that we will add future
+ * protocols, while retaining ABI compatibility, (totally overlooking
+ * the fact that no code for name-resolution supports that) it is
+ * usually defined so it can cope with 128 byte addresses.
+ *
+ * Does that ring a bell ?
+ *
+ * Only, not quite:  Remember that all APIs require you to track
+ * the address and the length separately, so you only get the
+ * size of the specific protocols sockaddr_${whatever} from API
+ * functions, not a full sockaddr_storage, and besides the
+ * prototype for the KPI is still "struct sockaddr *", so you
+ * cannot gain C type-safety back by using sockaddr_storage
+ * as the "generic network address" type.
+ *
+ * So we have come full circle, while causing maximum havoc along
+ * the way and for the forseeable future.
+ *
+ * Do I need to tell you that static code analysis tools have a
+ * really hard time coping with this, and that they give a lot of
+ * false negatives which confuse people ?
+ *
+ * I have decided to try to contain this crap in this single
+ * source-file, with only minimum leakage into the rest of Varnish,
+ * which will only know of pointers to "struct suckaddr", the naming
+ * of which is my of the historical narrative above.
+ *
+ * And you don't need to take my word for this, you can see it all
+ * in various #include files on your own system.   If you are on
+ * a Solaris derivative, don't miss the beautiful horror hidden in the
+ * variant definition of IPv6 addresses between kernel and userland.
+ *
+ */
+
+struct suckaddr {
+	union {
+		struct sockaddr		sa;
+		struct sockaddr_in	sa4;
+		struct sockaddr_in6	sa6;
+	};
+};
+
+const int vsa_suckaddr_len = sizeof(struct suckaddr);
+
+/*
+ * This VRT interface is for the VCC generated ACL code, which needs
+ * to know the address family and a pointer to the actual address.
+ */
+
+int
+VRT_VSA_GetPtr(const struct suckaddr *sua, const unsigned char ** dst)
+{
+	AN(dst);
+	if (sua == NULL)
+		return (-1);
+
+	switch(sua->sa.sa_family) {
+		case PF_INET:
+			assert(sua->sa.sa_family == sua->sa4.sin_family);
+			*dst = (const unsigned char *)&sua->sa4.sin_addr;
+			return (sua->sa4.sin_family);
+		case PF_INET6:
+			assert(sua->sa.sa_family == sua->sa6.sin6_family);
+			*dst = (const unsigned char *)&sua->sa6.sin6_addr;
+			return (sua->sa6.sin6_family);
+		default:
+			*dst = NULL;
+			return (-1);
+	}
+}
 
 int
 VSA_Sane(const void *s)
diff --git a/lib/libvcc/vcc_acl.c b/lib/libvcc/vcc_acl.c
index 433d848..182dbec 100644
--- a/lib/libvcc/vcc_acl.c
+++ b/lib/libvcc/vcc_acl.c
@@ -320,34 +320,6 @@ vcc_acl_entry(struct vcc *tl)
  * Emit a function to match the ACL we have collected
  */
 
-/*
- * XXX: this is semi-silly.  We try really hard to not depend in the
- * XXX: systems include files while compiling VCL, but we need to know
- * XXX: the size of the sa_familiy member.
- * XXX: FlexeLint complains about these antics, so isolate it in a
- * XXX: separate function.
- */
-
-/*lint -save -e506 -e774 -e550 */
-static void
-c_is_a_silly_language(const struct vcc *tl)
-{
-	struct sockaddr sa;
-
-	assert(sizeof (unsigned char) == 1);
-	assert(sizeof (unsigned short) == 2);
-	assert(sizeof (unsigned int) == 4);
-	if (sizeof sa.sa_family == 1)
-		Fh(tl, 0, "\tunsigned char fam;\n");
-	else if (sizeof sa.sa_family == 2)
-		Fh(tl, 0, "\tunsigned short fam;\n");
-	else if (sizeof sa.sa_family == 4)
-		Fh(tl, 0, "\tunsigned int fam;\n");
-	else
-		assert(0 == __LINE__);
-}
-/*lint -restore */
-
 static void
 vcc_acl_emit(const struct vcc *tl, const char *acln, int anon)
 {
@@ -357,21 +329,14 @@ vcc_acl_emit(const struct vcc *tl, const char *acln, int anon)
 	const char *oc;
 
 	Fh(tl, 0, "\nstatic int\n");
-	Fh(tl, 0, "match_acl_%s_%s(const struct vrt_ctx *ctx, const void *p)\n",
+	Fh(tl, 0, "match_acl_%s_%s(const struct vrt_ctx *ctx, const VCL_IP p)\n",
 	    anon ? "anon" : "named", acln);
 	Fh(tl, 0, "{\n");
 	Fh(tl, 0, "\tconst unsigned char *a;\n");
-	c_is_a_silly_language(tl);
-
+	Fh(tl, 0, "\tint fam;\n");
 	Fh(tl, 0, "\n");
-	Fh(tl, 0, "\ta = p;\n");
-	Fh(tl, 0, "\tVRT_memmove(&fam, a + %zd, sizeof fam);\n",
-	    offsetof(struct sockaddr, sa_family));
-	Fh(tl, 0, "\tif (fam == %d)\n", PF_INET);
-	Fh(tl, 0, "\t\ta += %zd;\n", offsetof(struct sockaddr_in, sin_addr));
-	Fh(tl, 0, "\telse if (fam == %d)\n", PF_INET6);
-	Fh(tl, 0, "\t\ta += %zd;\n", offsetof(struct sockaddr_in6, sin6_addr));
-	Fh(tl, 0, "\telse {\n");
+	Fh(tl, 0, "\tfam = VRT_VSA_GetPtr(p, &a);\n");
+	Fh(tl, 0, "\tif (fam < 0) {\n");
 	Fh(tl, 0, "\t\tVRT_acl_log(ctx, \"NO_FAM %s\");\n", acln);
 	Fh(tl, 0, "\t\treturn(0);\n");
 	Fh(tl, 0, "\t}\n\n");



More information about the varnish-commit mailing list