From 07d2b4c1af1c2cf210e73245ad3b6f8f7d07dc10 Mon Sep 17 00:00:00 2001 From: phk Date: Sat, 19 Jul 2008 09:24:07 +0000 Subject: [PATCH] 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: svn+ssh://projects.linpro.no/svn/varnish/trunk@2955 d4fa192b-c00b-0410-8231-f00ffab90ce4 --- varnish-cache/bin/varnishd/cache.h | 2 +- varnish-cache/bin/varnishd/cache_hash.c | 2 +- varnish-cache/bin/varnishd/common.h | 6 + varnish-cache/bin/varnishd/flint.lnt | 1 + varnish-cache/bin/varnishd/hash_classic.c | 25 ++-- varnish-cache/bin/varnishd/hash_simple_list.c | 10 +- varnish-cache/bin/varnishd/hash_slinger.h | 4 +- varnish-cache/bin/varnishd/mgt.h | 4 - varnish-cache/bin/varnishd/stevedore.c | 45 ++----- varnish-cache/bin/varnishd/stevedore.h | 6 +- varnish-cache/bin/varnishd/storage_file.c | 127 ++++++++---------- varnish-cache/bin/varnishd/storage_malloc.c | 43 +++--- varnish-cache/bin/varnishd/varnishd.c | 119 +++++++++++----- 13 files changed, 206 insertions(+), 188 deletions(-) diff --git a/varnish-cache/bin/varnishd/cache.h b/varnish-cache/bin/varnishd/cache.h index 77d89d15..2b058d79 100644 --- a/varnish-cache/bin/varnishd/cache.h +++ b/varnish-cache/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); diff --git a/varnish-cache/bin/varnishd/cache_hash.c b/varnish-cache/bin/varnishd/cache_hash.c index 99a05268..cfd03e0c 100644 --- a/varnish-cache/bin/varnishd/cache_hash.c +++ b/varnish-cache/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; diff --git a/varnish-cache/bin/varnishd/common.h b/varnish-cache/bin/varnishd/common.h index 81a113a3..d8f1347a 100644 --- a/varnish-cache/bin/varnishd/common.h +++ b/varnish-cache/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); diff --git a/varnish-cache/bin/varnishd/flint.lnt b/varnish-cache/bin/varnishd/flint.lnt index 9ef0e4cf..8140aee6 100644 --- a/varnish-cache/bin/varnishd/flint.lnt +++ b/varnish-cache/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 diff --git a/varnish-cache/bin/varnishd/hash_classic.c b/varnish-cache/bin/varnishd/hash_classic.c index fac93f8c..bd2841c1 100644 --- a/varnish-cache/bin/varnishd/hash_classic.c +++ b/varnish-cache/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, }; diff --git a/varnish-cache/bin/varnishd/hash_simple_list.c b/varnish-cache/bin/varnishd/hash_simple_list.c index 7d13cb27..a86ac910 100644 --- a/varnish-cache/bin/varnishd/hash_simple_list.c +++ b/varnish-cache/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, }; diff --git a/varnish-cache/bin/varnishd/hash_slinger.h b/varnish-cache/bin/varnishd/hash_slinger.h index 4df2dc11..16dec690 100644 --- a/varnish-cache/bin/varnishd/hash_slinger.h +++ b/varnish-cache/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; diff --git a/varnish-cache/bin/varnishd/mgt.h b/varnish-cache/bin/varnishd/mgt.h index 10af2f8f..088f87e5 100644 --- a/varnish-cache/bin/varnishd/mgt.h +++ b/varnish-cache/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 { \ diff --git a/varnish-cache/bin/varnishd/stevedore.c b/varnish-cache/bin/varnishd/stevedore.c index 9e40714a..81971c4c 100644 --- a/varnish-cache/bin/varnishd/stevedore.c +++ b/varnish-cache/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); diff --git a/varnish-cache/bin/varnishd/stevedore.h b/varnish-cache/bin/varnishd/stevedore.h index 06870b56..7f5df8ef 100644 --- a/varnish-cache/bin/varnishd/stevedore.h +++ b/varnish-cache/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); diff --git a/varnish-cache/bin/varnishd/storage_file.c b/varnish-cache/bin/varnishd/storage_file.c index 2df21fd8..fb6ffde4 100644 --- a/varnish-cache/bin/varnishd/storage_file.c +++ b/varnish-cache/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 diff --git a/varnish-cache/bin/varnishd/storage_malloc.c b/varnish-cache/bin/varnishd/storage_malloc.c index eb632431..75e6d914 100644 --- a/varnish-cache/bin/varnishd/storage_malloc.c +++ b/varnish-cache/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, }; diff --git a/varnish-cache/bin/varnishd/varnishd.c b/varnish-cache/bin/varnishd/varnishd.c index 2c67bf3d..5171561e 100644 --- a/varnish-cache/bin/varnishd/varnishd.c +++ b/varnish-cache/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); -- 2.39.5