Wash & cleaning of the -h and -s argument handling.

No functional changes.

Add ARGV_ERR() macro for reporting argument trouble.

Do selection of method in varnishd.c, using generic choice function.

Split subarguments into argc+argv at commans, and pass those down to
avoid repetitive and bogus string munging.

Catch more error conditions with respect to subarguments.

Add mini_obj magics to stevedores and slingers.

Generally tidy up along the way.


git-svn-id: http://www.varnish-cache.org/svn/trunk/varnish-cache@2955 d4fa192b-c00b-0410-8231-f00ffab90ce4

bin/varnishd/cache.h

@@ -464,7 +464,7 @@ void HSH_Prealloc(struct sess *sp);
 int HSH_Compare(const struct sess *sp, const struct objhead *o);
 void HSH_Copy(const struct sess *sp, const struct objhead *o);
 struct object *HSH_Lookup(struct sess *sp);
-void HSH_Unbusy(struct sess *sp);
+void HSH_Unbusy(const struct sess *sp);
 void HSH_Ref(struct object *o);
 void HSH_Deref(struct object *o);
 double HSH_Grace(double g);

bin/varnishd/cache_hash.c

@@ -302,7 +302,7 @@ hsh_rush(struct objhead *oh)
 }
 
 void
-HSH_Unbusy(struct sess *sp)
+HSH_Unbusy(const struct sess *sp)
 {
 	struct object *o;
 	struct objhead *oh;

bin/varnishd/common.h

@@ -58,3 +58,9 @@ int TCP_connect(int s, const struct sockaddr *name, socklen_t namelen, int msec)
 
 /* Really belongs in mgt.h, but storage_file chokes on both */
 void mgt_child_inherit(int fd, const char *what);
+
+#define ARGV_ERR(...)						\
+	do {							\
+		fprintf(stderr, "Error: " __VA_ARGS__);		\
+		exit(2);					\
+	} while (0);

bin/varnishd/flint.lnt

@@ -59,6 +59,7 @@
 -emacro(827, assert)	// loop not reachable
 -emacro(774, assert)	// booelan always true
 -emacro(774, HTTPH)	// always false
+-emacro(527, ARGV_ERR)	// unreachable
 
 // cache.h
 -emacro(506, INCOMPL) // Constant value Boolean

bin/varnishd/hash_classic.c

@@ -68,15 +68,19 @@ static struct hcl_hd		*hcl_head;
  * The ->init method allows the management process to pass arguments
  */
 
-static int
-hcl_init(const char *p)
+static void
+hcl_init(int ac, char * const *av)
 {
 	int i;
 	unsigned u;
 
-	i = sscanf(p, "%u", &u);
+	if (ac == 0)
+		return;
+	if (ac > 1)
+		ARGV_ERR("(-hclassic) too many arguments\n");
+	i = sscanf(av[0], "%u", &u);
 	if (i <= 0 || u == 0)
-		return (0);
+		return;
 	if (u > 2 && !(u & (u - 1))) {
 		fprintf(stderr,
 		    "NOTE:\n"
@@ -88,7 +92,7 @@ hcl_init(const char *p)
 	}
 	hcl_nhash = u;
 	fprintf(stderr, "Classic hash: %u buckets\n", hcl_nhash);
-	return (0);
+	return;
 }
 
 /*--------------------------------------------------------------------
@@ -245,9 +249,10 @@ hcl_deref(const struct objhead *oh)
 /*--------------------------------------------------------------------*/
 
 struct hash_slinger hcl_slinger = {
-	"classic",
-	hcl_init,
-	hcl_start,
-	hcl_lookup,
-	hcl_deref,
+	.magic	= 	SLINGER_MAGIC,
+	.name	=	"classic",
+	.init	=	hcl_init,
+	.start	=	hcl_start,
+	.lookup =	hcl_lookup,
+	.deref	=	hcl_deref,
 };

bin/varnishd/hash_simple_list.c

@@ -139,9 +139,9 @@ hsl_deref(const struct objhead *obj)
 /*--------------------------------------------------------------------*/
 
 struct hash_slinger hsl_slinger = {
-	"simple_list",
-	NULL,
-	hsl_start,
-	hsl_lookup,
-	hsl_deref,
+	.magic	=	SLINGER_MAGIC,
+	.name	=	"simple",
+	.start	=	hsl_start,
+	.lookup =	hsl_lookup,
+	.deref 	=	hsl_deref,
 };

bin/varnishd/hash_slinger.h

@@ -31,12 +31,14 @@
 
 struct sess;
 
-typedef int hash_init_f(const char *);
+typedef void hash_init_f(int ac, char * const *av);
 typedef void hash_start_f(void);
 typedef struct objhead *hash_lookup_f(const struct sess *sp, struct objhead *nobj);
 typedef int hash_deref_f(const struct objhead *obj);
 
 struct hash_slinger {
+	unsigned		magic;
+#define SLINGER_MAGIC		0x1b720cba
 	const char		*name;
 	hash_init_f		*init;
 	hash_start_f		*start;

bin/varnishd/mgt.h

@@ -63,10 +63,6 @@ int mgt_push_vcls_and_start(unsigned *status, char **p);
 int mgt_has_vcl(void);
 extern char *mgt_cc_cmd;
 
-#include "hash_slinger.h"
-
-extern struct hash_slinger hsl_slinger;
-extern struct hash_slinger hcl_slinger;
 
 #define REPORT0(pri, fmt)				\
 	do {						\

bin/varnishd/stevedore.c

@@ -37,9 +37,6 @@
 #include "cache.h"
 #include "stevedore.h"
 
-extern struct stevedore sma_stevedore;
-extern struct stevedore smf_stevedore;
-
 static VTAILQ_HEAD(, stevedore)	stevedores =
     VTAILQ_HEAD_INITIALIZER(stevedores);
 
@@ -95,47 +92,21 @@ STV_free(const struct storage *st)
 	st->stevedore->free(st);
 }
 
-static int
-cmp_storage(const struct stevedore *s, const char *p, const char *q)
-{
-	unsigned u;
-
-	u = pdiff(p, q);
-	if (strlen(s->name) != u)
-		return (1);
-	if (strncmp(s->name, p, u))
-		return (1);
-	return (0);
-}
-
 void
-STV_add(const char *spec)
+STV_add(const struct stevedore *stv2, int ac, char * const *av)
 {
-	const char *p, *q;
 	struct stevedore *stv;
 
-	p = strchr(spec, ',');
-	if (p == NULL)
-		q = p = strchr(spec, '\0');
-	else
-		q = p + 1;
-	xxxassert(p != NULL);
-	xxxassert(q != NULL);
-
-	stv = malloc(sizeof *stv);
+	CHECK_OBJ_NOTNULL(stv2, STEVEDORE_MAGIC);
+	ALLOC_OBJ(stv, STEVEDORE_MAGIC);
 	AN(stv);
 
-	if (!cmp_storage(&sma_stevedore, spec, p)) {
-		*stv = sma_stevedore;
-	} else if (!cmp_storage(&smf_stevedore, spec, p)) {
-		*stv = smf_stevedore;
-	} else {
-		fprintf(stderr, "Unknown storage method \"%.*s\"\n",
-		    (int)(p - spec), spec);
-		exit (2);
-	}
+	*stv = *stv2;
+
 	if (stv->init != NULL)
-		stv->init(stv, q);
+		stv->init(stv, ac, av);
+	else if (ac != 0) 
+		ARGV_ERR("(-s%s) too many arguments\n", stv->name);
 
 	VTAILQ_INSERT_TAIL(&stevedores, stv, list);
 

bin/varnishd/stevedore.h

@@ -35,13 +35,15 @@ struct stevedore;
 struct sess;
 struct iovec;
 
-typedef void storage_init_f(struct stevedore *, const char *spec);
+typedef void storage_init_f(struct stevedore *, int ac, char * const *av);
 typedef void storage_open_f(const struct stevedore *);
 typedef struct storage *storage_alloc_f(struct stevedore *, size_t size);
 typedef void storage_trim_f(const struct storage *, size_t size);
 typedef void storage_free_f(const struct storage *);
 
 struct stevedore {
+	unsigned		magic;
+#define STEVEDORE_MAGIC		0x4baf43db
 	const char		*name;
 	storage_init_f		*init;	/* called by mgt process */
 	storage_open_f		*open;	/* called by cache process */
@@ -58,5 +60,5 @@ struct stevedore {
 struct storage *STV_alloc(struct sess *sp, size_t size);
 void STV_trim(const struct storage *st, size_t size);
 void STV_free(const struct storage *st);
-void STV_add(const char *spec);
+void STV_add(const struct stevedore *stv, int ac, char * const *av);
 void STV_open(void);

bin/varnishd/storage_file.c

@@ -108,7 +108,7 @@ struct smf {
 };
 
 struct smf_sc {
-	char			*filename;
+	const char		*filename;
 	int			fd;
 	unsigned		pagesize;
 	uintmax_t		filesize;
@@ -172,11 +172,8 @@ smf_calcsize(struct smf_sc *sc, const char *size, int newfile)
 	} else {
 		q = str2bytes(size, &l, fssize);
 
-		if (q != NULL) {
-			fprintf(stderr,
-			    "Error: (-sfile) size \"%s\": %s\n", size, q);
-			exit (2);
-		}
+		if (q != NULL)
+			ARGV_ERR("(-sfile) size \"%s\": %s\n", size, q);
 	}
 
 	/*
@@ -206,12 +203,9 @@ smf_calcsize(struct smf_sc *sc, const char *size, int newfile)
 	/* round down to multiple of filesystem blocksize or pagesize */
 	l -= (l % bs);
 
-	if (l < MINPAGES * (uintmax_t)sc->pagesize) {
-		fprintf(stderr,
-		    "Error: size too small, at least %ju needed\n",
+	if (l < MINPAGES * (uintmax_t)sc->pagesize)
+		ARGV_ERR("size too small, at least %ju needed\n",
 		    (uintmax_t)MINPAGES * sc->pagesize);
-		exit (2);
-	}
 
 	if (sizeof(void *) == 4 && l > INT32_MAX) { /*lint !e506 !e774 */
 		fprintf(stderr,
@@ -238,15 +232,33 @@ smf_initfile(struct smf_sc *sc, const char *size, int newfile)
 	/* XXX: force block allocation here or in open ? */
 }
 
+static char default_size[] = "50%";
+static char default_filename[] = ".";
+
 static void
-smf_init(struct stevedore *parent, const char *spec)
+smf_init(struct stevedore *parent, int ac, char * const *av)
 {
-	char *size;
-	char *p, *q;
+	const char *size, *fn;
+	char *q, *p;
 	struct stat st;
 	struct smf_sc *sc;
 	unsigned u;
 
+	AZ(av[ac]);
+
+	fn = default_filename;
+	size = default_size;
+
+	if (ac > 2)
+		ARGV_ERR("(-sfile) too many arguments\n");
+	if (ac > 0 && *av[0] != '\0')
+		fn = av[0];
+	if (ac > 1 && *av[1] != '\0')
+		size = av[1];
+
+	AN(fn);
+	AN(size);
+
 	sc = calloc(sizeof *sc, 1);
 	XXXAN(sc);
 	VTAILQ_INIT(&sc->order);
@@ -257,82 +269,52 @@ smf_init(struct stevedore *parent, const char *spec)
 
 	parent->priv = sc;
 
-	/* If no size specified, use 50% of filesystem free space */
-	if (spec == NULL || *spec == '\0')
-		asprintf(&p, ".,50%%");
-	else if (strchr(spec, ',') == NULL)
-		asprintf(&p, "%s,", spec);
-	else
-		p = strdup(spec);
-	XXXAN(p);
-	size = strchr(p, ',');
-	XXXAN(size);
-
-	*size++ = '\0';
-
 	/* try to create a new file of this name */
-	sc->fd = open(p, O_RDWR | O_CREAT | O_EXCL, 0600);
+	sc->fd = open(fn, O_RDWR | O_CREAT | O_EXCL, 0600);
 	if (sc->fd >= 0) {
-		sc->filename = p;
+		sc->filename = fn;
 		mgt_child_inherit(sc->fd, "storage_file");
 		smf_initfile(sc, size, 1);
 		return;
 	}
 
 	/* it must exist then */
-	if (stat(p, &st)) {
-		fprintf(stderr,
-		    "Error: (-sfile) \"%s\" "
-		    "does not exist and could not be created\n", p);
-		exit (2);
-	}
+	if (stat(fn, &st))
+		ARGV_ERR("(-sfile) \"%s\" "
+		    "does not exist and could not be created\n", fn);
 
 	/* and it should be a file or directory */
-	if (!(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode))) {
-		fprintf(stderr,
-		    "Error: (-sfile) \"%s\" "
-		    "is neither file nor directory\n", p);
-		exit (2);
-	}
+	if (!(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode)))
+		ARGV_ERR("(-sfile) \"%s\" is neither file nor directory\n", fn);
 
 	if (S_ISREG(st.st_mode)) {
-		sc->fd = open(p, O_RDWR);
-		if (sc->fd < 0) {
-			fprintf(stderr,
-			    "Error: (-sfile) \"%s\" "
-			    "could not open (%s)\n", p, strerror(errno));
-			exit (2);
-		}
+		sc->fd = open(fn, O_RDWR);
+		if (sc->fd < 0) 
+			ARGV_ERR("(-sfile) \"%s\" could not open (%s)\n",
+			    fn, strerror(errno));
 		AZ(fstat(sc->fd, &st));
-		if (!S_ISREG(st.st_mode)) {
-			fprintf(stderr,
-			    "Error: (-sfile) \"%s\" "
-			    "was not a file after opening\n", p);
-			exit (2);
-		}
-		sc->filename = p;
+		if (!S_ISREG(st.st_mode))
+			ARGV_ERR("(-sfile) \"%s\" "
+			    "was not a file after opening\n", fn);
+		sc->filename = fn;
 		mgt_child_inherit(sc->fd, "storage_file");
 		smf_initfile(sc, size, 0);
 		return;
 	}
 
-
-	asprintf(&q, "%s/varnish.XXXXXX", p);
+	asprintf(&q, "%s/varnish.XXXXXX", fn);
 	XXXAN(q);
 	sc->fd = mkstemp(q);
-	if (sc->fd < 0) {
-		fprintf(stderr,
-		    "Error: (-sfile) \"%s\" "
-		    "mkstemp(%s) failed (%s)\n", p, q, strerror(errno));
-		exit (2);
-	}
+	if (sc->fd < 0)
+		ARGV_ERR("(-sfile) \"%s\" "
+		    "mkstemp(%s) failed (%s)\n", fn, q, strerror(errno));
 	AZ(unlink(q));
-	asprintf(&sc->filename, "%s (unlinked)", q);
-	XXXAN(sc->filename);
+	asprintf(&p, "%s (unlinked)", q);
+	XXXAN(p);
+	sc->filename = p;
 	free(q);
 	smf_initfile(sc, size, 1);
 	mgt_child_inherit(sc->fd, "storage_file");
-	free(p);
 }
 
 /*--------------------------------------------------------------------
@@ -705,12 +687,13 @@ smf_free(const struct storage *s)
 /*--------------------------------------------------------------------*/
 
 struct stevedore smf_stevedore = {
-	.name =		"file",
-	.init =		smf_init,
-	.open =		smf_open,
-	.alloc =	smf_alloc,
-	.trim =		smf_trim,
-	.free =		smf_free,
+	.magic	=	STEVEDORE_MAGIC,
+	.name	=	"file",
+	.init	=	smf_init,
+	.open	=	smf_open,
+	.alloc	=	smf_alloc,
+	.trim	=	smf_trim,
+	.free	=	smf_free,
 };
 
 #ifdef INCLUDE_TEST_DRIVER

bin/varnishd/storage_malloc.c

@@ -123,26 +123,26 @@ sma_trim(const struct storage *s, size_t size)
 }
 
 static void
-sma_init(struct stevedore *parent, const char *spec)
+sma_init(struct stevedore *parent, int ac, char * const *av)
 {
 	const char *e;
 	uintmax_t u;
 
 	(void)parent;
-	if (spec != NULL && *spec != '\0') {
-		e = str2bytes(spec, &u, 0);
-		if (e != NULL) {
-			fprintf(stderr,
-			    "Error: (-smalloc) size \"%s\": %s\n", spec, e);
-			exit(2);
-		}
-		if ((u != (uintmax_t)(size_t)u)) {
-			fprintf(stderr,
-			    "Error: (-smalloc) size \"%s\": too big\n", spec);
-			exit(2);
-		}
-		sma_max = u;
-	}
+
+	AZ(av[ac]);
+	if (ac > 1) 
+		ARGV_ERR("(-smalloc) too many arguments\n");
+
+	if (ac == 0 || *av[0] == '\0')
+		 return;
+
+	e = str2bytes(av[0], &u, 0);
+	if (e != NULL)
+		ARGV_ERR("(-smalloc) size \"%s\": %s\n", av[0], e);
+	if ((u != (uintmax_t)(size_t)u)) 
+		ARGV_ERR("(-smalloc) size \"%s\": too big\n", av[0]);
+	sma_max = u;
 }
 
 static void
@@ -153,10 +153,11 @@ sma_open(const struct stevedore *st)
 }
 
 struct stevedore sma_stevedore = {
-	.name =		"malloc",
-	.init =		sma_init,
-	.open =		sma_open,
-	.alloc =	sma_alloc,
-	.free =		sma_free,
-	.trim =		sma_trim,
+	.magic	=	STEVEDORE_MAGIC,
+	.name	=	"malloc",
+	.init	=	sma_init,
+	.open	=	sma_open,
+	.alloc	=	sma_alloc,
+	.free	=	sma_free,
+	.trim	=	sma_trim,
 };

bin/varnishd/varnishd.c

@@ -67,6 +67,7 @@
 #include "shmlog.h"
 #include "heritage.h"
 #include "mgt.h"
+#include "hash_slinger.h"
 #include "stevedore.h"
 
 /* INFTIM indicates an infinite timeout for poll(2) */
@@ -79,47 +80,97 @@ volatile struct params *params;
 
 /*--------------------------------------------------------------------*/
 
-static int
-cmp_hash(const struct hash_slinger *s, const char *p, const char *q)
+struct choice {
+	const char	*name;
+	void		*ptr;
+};
+
+static void *
+pick(const struct choice *cp, const char *which, const char *kind)
 {
-	if (strlen(s->name) != (q - p))
-		return (1);
-	if (strncmp(s->name, p, (q - p)))
-		return (1);
-	return (0);
+
+	for(; cp->name != NULL; cp++) {
+		if (!strcmp(cp->name, which))
+			return (cp->ptr);
+	}
+	ARGV_ERR("Unknown %s method \"%s\"\n", kind, which);
 }
 
+/*--------------------------------------------------------------------*/
+
+extern struct stevedore sma_stevedore;
+extern struct stevedore smf_stevedore;
+
+static struct choice stv_choice[] = {
+	{ "file",	&smf_stevedore },
+	{ "malloc",	&sma_stevedore },
+	{ NULL,		NULL }
+};
+
 static void
-setup_hash(const char *s_arg)
+setup_storage(const char *spec)
 {
-	const char *p, *q;
+	char **av;
+	void *priv;
+	int ac;
+
+	av = ParseArgv(spec, ARGV_COMMA);
+
+	if (av[0] != NULL) 
+		ARGV_ERR("%s\n", av[0]);
+
+	if (av[1] == NULL)
+		ARGV_ERR("-s argument is empty\n");
+
+	for (ac = 0; av[ac + 2] != NULL; ac++)
+		continue;
+
+	priv = pick(stv_choice, av[1], "storage");
+	AN(priv);
+
+	STV_add(priv, ac, av + 2);
+
+	/* We do not free av, to make life simpler for stevedores */
+}
+
+/*--------------------------------------------------------------------*/
+
+extern struct hash_slinger hsl_slinger;
+extern struct hash_slinger hcl_slinger;
+
+static struct choice hsh_choice[] = {
+	{ "classic",		&hcl_slinger },
+	{ "simple",		&hsl_slinger },
+	{ "simple_list",	&hsl_slinger },	/* backwards compat */
+	{ NULL,			NULL }
+};
+
+static void
+setup_hash(const char *h_arg)
+{
+	char **av;
+	int ac;
 	struct hash_slinger *hp;
 
-	p = strchr(s_arg, ',');
-	if (p == NULL)
-		q = p = strchr(s_arg, '\0');
-	else
-		q = p + 1;
-	xxxassert(p != NULL);
-	xxxassert(q != NULL);
-	if (!cmp_hash(&hcl_slinger, s_arg, p)) {
-		hp = &hcl_slinger;
-	} else if (!cmp_hash(&hsl_slinger, s_arg, p)) {
-		hp = &hsl_slinger;
-	} else {
-		fprintf(stderr, "Unknown hash method \"%.*s\"\n",
-		    (int)(p - s_arg), s_arg);
-		exit (2);
-	}
+	av = ParseArgv(h_arg, ARGV_COMMA);
+
+	if (av[0] != NULL) 
+		ARGV_ERR("%s\n", av[0]);
+
+	if (av[1] == NULL)
+		ARGV_ERR("-h argument is empty\n");
+
+	for (ac = 0; av[ac + 2] != NULL; ac++)
+		continue;
+
+	hp = pick(hsh_choice, av[1], "hash");
+	CHECK_OBJ_NOTNULL(hp, SLINGER_MAGIC);
 	heritage.hash = hp;
-	if (hp->init != NULL) {
-		if (hp->init(q))
-			exit (1);
-	} else if (*q) {
-		fprintf(stderr, "Hash method \"%s\" takes no arguments\n",
+	if (hp->init != NULL)
+		hp->init(ac, av + 2);
+	else if (ac > 0)
+		ARGV_ERR("Hash method \"%s\" takes no arguments\n",
 		    hp->name);
-		exit (1);
-	}
 }
 
 /*--------------------------------------------------------------------*/
@@ -436,7 +487,7 @@ main(int argc, char *argv[])
 			break;
 		case 's':
 			s_arg_given = 1;
-			STV_add(optarg);
+			setup_storage(optarg);
 			break;
 		case 't':
 			MCF_ParamSet(cli, "default_ttl", optarg);
@@ -538,7 +589,7 @@ main(int argc, char *argv[])
 		exit (0);
 
 	if (!s_arg_given)
-		STV_add(s_arg);
+		setup_storage(s_arg);
 
 	setup_hash(h_arg);