More parameter code polishing.

bin/varnishd/mgt/mgt.h

@@ -75,6 +75,8 @@ const void *pick(const struct choice *cp, const char *which, const char *kind);
 void MCF_InitParams(struct cli *);
 void MCF_CollectParams(void);
 void MCF_SetDefault(const char *param, const char *def);
+void MCF_SetMinimum(const char *param, const char *def);
+void MCF_SetMaximum(const char *param, const char *def);
 void MCF_ParamSet(struct cli *, const char *param, const char *val);
 void MCF_ParamProtect(struct cli *, const char *arg);
 void MCF_DumpRstParam(void);

bin/varnishd/mgt/mgt_main.c

@@ -339,6 +339,42 @@ make_secret(const char *dirname)
 
 /*--------------------------------------------------------------------*/
 
+static char stackmin[20];
+
+static void
+init_params(struct cli *cli)
+{
+	ssize_t low;
+
+	MCF_CollectParams();
+
+	/* If we have nobody/nogroup, use them as defaults */
+	if (getpwnam("nobody") != NULL)
+		MCF_SetDefault("user", "nobody");
+	if (getgrnam("nogroup") != NULL)
+		MCF_SetDefault("group", "nogroup");
+
+	if (sizeof(void *) < 8) {
+		/*
+		 * Adjust default parameters for 32 bit systems to conserve
+		 * VM space.
+		 */
+		MCF_SetDefault("workspace_client", "24k");
+		MCF_SetDefault("workspace_backend", "16k");
+		MCF_SetDefault("http_resp_size", "8k");
+		MCF_SetDefault("http_req_size", "12k");
+		MCF_SetDefault("gzip_buffer", "4k");
+	}
+
+	low = sysconf(_SC_THREAD_STACK_MIN);
+	bprintf(stackmin, "%jd", (intmax_t)low);
+	MCF_SetMinimum("thread_pool_stack", stackmin);
+
+	MCF_InitParams(cli);
+}
+
+/*--------------------------------------------------------------------*/
+
 int
 main(int argc, char * const *argv)
 {
@@ -388,6 +424,7 @@ main(int argc, char * const *argv)
 	/* for ASSERT_MGT() */
 	mgt_pid = getpid();
 
+
 	/*
 	 * Run in UTC timezone, on the off-chance that this operating
 	 * system does not have a timegm() function, and translates
@@ -415,28 +452,7 @@ main(int argc, char * const *argv)
 
 	VTAILQ_INIT(&heritage.socks);
 
-	MCF_CollectParams();
-
-	/* If we have nobody/nogroup, use them as defaults */
-	if (getpwnam("nobody") != NULL)
-		MCF_SetDefault("user", "nobody");
-	if (getgrnam("nogroup") != NULL)
-		MCF_SetDefault("group", "nogroup");
-
-	if (sizeof(void *) < 8) {
-		/*
-		 * Adjust default parameters for 32 bit systems to conserve
-		 * VM space.
-		 */
-		MCF_SetDefault("workspace_client", "24k");
-		MCF_SetDefault("workspace_backend", "16k");
-		MCF_SetDefault("http_resp_size", "8k");
-		MCF_SetDefault("http_req_size", "12k");
-		MCF_SetDefault("gzip_buffer", "4k");
-	}
-
-	MCF_InitParams(cli);
-
+	init_params(cli);
 	cli_check(cli);
 
 	while ((o = getopt(argc, argv,

bin/varnishd/mgt/mgt_param.c

@@ -30,18 +30,15 @@
 #include "config.h"
 
 #include <ctype.h>
-#include <math.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <unistd.h>
 
 #include "mgt/mgt.h"
 #include "common/heritage.h"
 #include "common/params.h"
 
 #include "mgt/mgt_param.h"
-#include "waiter/waiter.h"
 #include "vav.h"
 #include "vcli.h"
 #include "vcli_common.h"
@@ -468,13 +465,36 @@ MCF_CollectParams(void)
 /*--------------------------------------------------------------------*/
 
 void
-MCF_SetDefault(const char *param, const char *def)
+MCF_SetDefault(const char *param, const char *new_def)
 {
 	struct parspec *pp;
 
 	pp = mcf_findpar(param, NULL);
 	AN(pp);
-	pp->def = def;
+	pp->def = new_def;
+	AN(pp->def);
+}
+
+void
+MCF_SetMinimum(const char *param, const char *new_min)
+{
+	struct parspec *pp;
+
+	pp = mcf_findpar(param, NULL);
+	AN(pp);
+	pp->min = new_min;
+	AN(pp->min);
+}
+
+void
+MCF_SetMaximum(const char *param, const char *new_max)
+{
+	struct parspec *pp;
+
+	pp = mcf_findpar(param, NULL);
+	AN(pp);
+	pp->max = new_max;
+	AN(pp->max);
 }
 
 /*--------------------------------------------------------------------*/

bin/varnishd/mgt/mgt_param_tweak.c

@@ -36,7 +36,6 @@
 #include <limits.h>
 #include <math.h>
 #include <pwd.h>
-#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -48,14 +47,9 @@
 #include "mgt/mgt_param.h"
 #include "waiter/waiter.h"
 #include "vav.h"
-#include "vcli.h"
-#include "vcli_common.h"
-#include "vcli_priv.h"
 #include "vnum.h"
 #include "vss.h"
 
-#include "mgt_cli.h"
-
 /*--------------------------------------------------------------------
  * Generic handling of double typed parameters
  */
@@ -156,10 +150,6 @@ int
 tweak_bool(struct vsb *vsb, const struct parspec *par, const char *arg)
 {
 	volatile unsigned *dest;
-	int mode = 0;
-
-	if (!strcmp(par->def, "off") || !strcmp(par->def, "on"))
-		mode = 1;
 
 	dest = par->priv;
 	if (arg != NULL) {
@@ -180,16 +170,11 @@ tweak_bool(struct vsb *vsb, const struct parspec *par, const char *arg)
 		else if (!strcasecmp(arg, "true"))
 			*dest = 1;
 		else {
-			VSB_printf(vsb, "%s",
-			    mode ?
-				"use \"on\" or \"off\"\n" :
-				"use \"true\" or \"false\"\n");
+			VSB_printf(vsb, "use \"on\" or \"off\"\n");
 			return (-1);
 		}
-	} else if (mode) {
-		VSB_printf(vsb, "%s", *dest ? "on" : "off");
 	} else {
-		VSB_printf(vsb, "%s", *dest ? "true" : "false");
+		VSB_printf(vsb, "%s", *dest ? "on" : "off");
 	}
 	return (0);
 }

bin/varnishd/mgt/mgt_pool.c

@@ -43,58 +43,48 @@
 #include "config.h"
 
 #include <stdio.h>
-#include <string.h>
-#include <unistd.h>
 
 #include "mgt/mgt.h"
 #include "common/params.h"
 
 #include "mgt/mgt_param.h"
 
-/*--------------------------------------------------------------------*/
-
-static int
-tweak_thread_pool_min(struct vsb *vsb, const struct parspec *par,
-    const char *arg)
-{
-
-	return (tweak_generic_uint(vsb, &mgt_param.wthread_min, arg,
-	    // par->min, mgt_param.wthread_max));
-	    par->min, NULL));
-}
-
 /*--------------------------------------------------------------------
- * This is utterly ridiculous:  POSIX does not guarantee that the
- * minimum thread stack size is a compile time constant.
- * XXX: "32bit" is a magic marker for 32bit systems.
+ * The min/max values automatically update the opposites appropriate
+ * limit, so they don't end up crossing.
  */
 
+static char min_val[20];
+static char max_val[20];
+
 static int
-tweak_stack_size(struct vsb *vsb, const struct parspec *par,
+tweak_thread_pool_min(struct vsb *vsb, const struct parspec *par,
     const char *arg)
 {
-	ssize_t low;
-
-	low = sysconf(_SC_THREAD_STACK_MIN);
+	struct vsb v2;
 
-	if (tweak_bytes(vsb, par, arg))
+	if (tweak_generic_uint(vsb, par->priv, arg, par->min, par->max))
 		return (-1);
-	if (mgt_param.wthread_stacksize < low)
-		mgt_param.wthread_stacksize = low;
+	AN(VSB_new(&v2, min_val, sizeof min_val, 0));
+	AZ(tweak_generic_uint(&v2, &mgt_param.wthread_min, NULL, NULL, NULL));
+	AZ(VSB_finish(&v2));
+	MCF_SetMinimum("thread_pool_max", min_val);
 	return (0);
 }
 
-/*--------------------------------------------------------------------*/
-
 static int
 tweak_thread_pool_max(struct vsb *vsb, const struct parspec *par,
     const char *arg)
 {
+	struct vsb v2;
 
-	(void)par;
-	return (tweak_generic_uint(vsb, &mgt_param.wthread_max, arg,
-	    //mgt_param.wthread_min, NULL));
-	    NULL, NULL));
+	if (tweak_generic_uint(vsb, par->priv, arg, par->min, par->max))
+		return (-1);
+	AN(VSB_new(&v2, max_val, sizeof max_val, 0));
+	AZ(tweak_generic_uint(&v2, &mgt_param.wthread_max, NULL, NULL, NULL));
+	AZ(VSB_finish(&v2));
+	MCF_SetMaximum("thread_pool_min", max_val);
+	return (0);
 }
 
 /*--------------------------------------------------------------------*/
@@ -114,8 +104,8 @@ struct parspec WRK_parspec[] = {
 		"restart to take effect.",
 		EXPERIMENTAL | DELAYED_EFFECT,
 		"2", "pools" },
-	{ "thread_pool_max", tweak_thread_pool_max, NULL,
-		"10", NULL,
+	{ "thread_pool_max", tweak_thread_pool_max, &mgt_param.wthread_max,
+		NULL, NULL,
 		"The maximum number of worker threads in each pool.\n"
 		"\n"
 		"Do not set this higher than you have to, since excess "
@@ -125,8 +115,8 @@ struct parspec WRK_parspec[] = {
 		"Minimum is 10 threads.",
 		DELAYED_EFFECT,
 		"5000", "threads" },
-	{ "thread_pool_min", tweak_thread_pool_min, NULL,
-		"10", NULL,
+	{ "thread_pool_min", tweak_thread_pool_min, &mgt_param.wthread_min,
+		NULL, NULL,
 		"The minimum number of worker threads in each pool.\n"
 		"\n"
 		"Increasing this may help ramp up faster from low load "
@@ -218,11 +208,11 @@ struct parspec WRK_parspec[] = {
 		EXPERIMENTAL,
 		"3", "requests per request" },
 	{ "thread_pool_stack",
-		tweak_stack_size, &mgt_param.wthread_stacksize,
-		"0", NULL,
+		tweak_bytes, &mgt_param.wthread_stacksize,
+		NULL, NULL,
 		"Worker thread stack size.\n"
-		"This is likely rounded up to a multiple of 4k by the kernel.\n"
-		"The kernel/OS has a lower limit which will be enforced.",
+		"This will likely be rounded up to a multiple of 4k"
+		" (or whatever the page_size might be) by the kernel.",
 		EXPERIMENTAL,
 		"48k", "bytes" },
 	{ NULL, NULL, NULL }

bin/varnishtest/tests/b00042.vtc

@@ -17,12 +17,17 @@ varnish v1 -clierr "106" "param.set prefer_ipv6 foobar"
 varnish v1 -clierr "106" "param.set http_max_hdr 0"
 varnish v1 -clierr "106" "param.set http_max_hdr 1000000"
 varnish v1 -clierr "106" "param.set workspace_thread 1b"
-varnish v1 -clierr "106" "param.set workspace_thread 1b"
+varnish v1 -clierr "106" "param.set workspace_thread 1m"
 varnish v1 -clierr "106" "param.set workspace_thread 1x"
 varnish v1 -clierr "106" "param.set workspace_thread x"
 varnish v1 -clierr "106" "param.set user ///"
 varnish v1 -clierr "106" "param.set user ///"
 varnish v1 -clierr "106" {param.set listen_address ""}
-varnish v1 -clierr "106" {param.set listen_address ",,"}
+varnish v1 -clierr "106" {param.set listen_address "&&"}
 varnish v1 -clierr "106" {param.set listen_address "\""}
 varnish v1 -clierr "106" {param.set pool_sess "\""}
+varnish v1 -clierr "200" {param.set thread_pool_max 110}
+varnish v1 -clierr "106" {param.set thread_pool_min 111}
+varnish v1 -clierr "200" {param.set thread_pool_min 51}
+varnish v1 -clierr "106" {param.set thread_pool_max 50}
+varnish v1 -clierr "200" {param.set thread_pool_max 51}