FlexeLint spotted a memory leak in Tollefs "admin-health" code, so I

started fixing that, and then I fixed another couple of nits and polished
the naming a bit and ...

Well, you know how it is.

Changed the backend matching function to take a call-back function
to eliminate the need for the memory which got leaked.

This meant that backend.list could also use it, if only the parsing
code accepted a blank input.

So I rewrote the parsing code to also allow empty names, so you can:
	backend.list (:80)
	backend.list (192.168)
etc.

And then I added a summary report of the probing, if it is happening,
and explicitly say that no probing is happening if it isn't.

bin/varnishd/cache_backend.c

@@ -259,10 +259,10 @@ vbe_Healthy(const struct vdi_simple *vs, const struct sess *sp)
 	backend = vs->backend;
 	CHECK_OBJ_NOTNULL(backend, BACKEND_MAGIC);
 
-	if (backend->admin_health == from_probe && !backend->healthy)
+	if (backend->admin_health == ah_probe && !backend->healthy)
 		return (0);
 
-	if (backend->admin_health == sick)
+	if (backend->admin_health == ah_sick)
 		return (0);
 
 	/* VRT/VCC sets threshold to UINT_MAX to mark that it's not
@@ -273,7 +273,7 @@ vbe_Healthy(const struct vdi_simple *vs, const struct sess *sp)
 	else
 		threshold = vs->vrt->saintmode_threshold;
 
-	if (backend->admin_health == healthy)
+	if (backend->admin_health == ah_healthy)
 		threshold = UINT_MAX;
 
 	/* Saintmode is disabled */

bin/varnishd/cache_backend.h

@@ -106,10 +106,11 @@ struct trouble {
  * An instance of a backend from a VCL program.
  */
 
-enum health_status {
-	healthy,
-	sick,
-	from_probe
+enum admin_health {
+	ah_invalid = 0,
+	ah_healthy,
+	ah_sick,
+	ah_probe
 };
 
 struct backend {
@@ -135,7 +136,7 @@ struct backend {
 
 	struct vbp_target	*probe;
 	unsigned		healthy;
-	enum health_status	admin_health;
+	enum admin_health	admin_health;
 	VTAILQ_HEAD(, trouble)	troublelist;
 
 	struct VSC_C_vbe	*vsc;
@@ -155,6 +156,7 @@ void VBE_DropRefLocked(struct backend *b);
 void VBP_Insert(struct backend *b, struct vrt_backend_probe const *p, const char *hosthdr);
 void VBP_Remove(struct backend *b, struct vrt_backend_probe const *p);
 void VBP_Use(const struct backend *b, const struct vrt_backend_probe const *p);
+void VBP_Summary(struct cli *cli, const struct vbp_target *vt);
 
 /* Init functions for directors */
 typedef void dir_init_f(struct cli *, struct director **, int , const void*);

bin/varnishd/cache_backend_cfg.c

@@ -32,6 +32,7 @@
 
 #include "config.h"
 
+#include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -48,6 +49,7 @@
 
 struct lock VBE_mtx;
 
+
 /*
  * The list of backends is not locked, it is only ever accessed from
  * the CLI thread, so there is no need.
@@ -83,6 +85,11 @@ VBE_Poll(void)
 
 	ASSERT_CLI();
 	VTAILQ_FOREACH_SAFE(b, &backends, list, b2) {
+		assert(
+			b->admin_health == ah_healthy ||
+			b->admin_health == ah_sick ||
+			b->admin_health == ah_probe
+		);
 		if (b->refcount == 0 && b->probe == NULL)
 			VBE_Nuke(b);
 	}
@@ -233,7 +240,7 @@ VBE_AddBackend(struct cli *cli, const struct vrt_backend *vb)
 	assert(b->ipv4 != NULL || b->ipv6 != NULL);
 
 	b->healthy = 1;
-	b->admin_health = from_probe;
+	b->admin_health = ah_probe;
 
 	VTAILQ_INSERT_TAIL(&backends, b, list);
 	VSC_C_main->n_backend++;
@@ -277,172 +284,227 @@ VRT_fini_dir(struct cli *cli, struct director *b)
 	b->priv = NULL;
 }
 
-/*--------------------------------------------------------------------*/
+/*---------------------------------------------------------------------
+ * String to admin_health
+ */
+
+static enum admin_health
+vbe_str2adminhealth(const char *wstate)
+{
+
+	if (strcasecmp(wstate, "healthy") == 0)
+		return(ah_healthy);
+	if (strcasecmp(wstate, "sick") == 0)
+		return(ah_sick);
+	if (strcmp(wstate, "auto") == 0)
+		return(ah_probe);
+	return (ah_invalid);
+}
+
+/*---------------------------------------------------------------------
+ * A general function for finding backends and doing things with them.
+ *
+ * Return -1 on match-argument parse errors.
+ *
+ * If the call-back function returns non-zero, the search is terminated
+ * and we relay that return value.
+ *
+ * Otherwise we return the number of matches.
+ */
+
+typedef int bf_func(struct cli *cli, struct backend *b, void *priv);
 
 static int
-backend_find(const char *matcher, struct backend **r, int n)
+backend_find(struct cli *cli, const char *matcher, bf_func *func, void *priv)
 {
 	struct backend *b;
-	char *vcl_name;
-	char *s;
-	char *match_ip = NULL;
-	char *match_port = NULL;
+	const char *s;
+	const char *name_b;
+	ssize_t name_l = 0;
+	const char *ip_b = NULL;
+	ssize_t ip_l = 0;
+	const char *port_b = NULL;
+	ssize_t port_l = 0;
 	int found = 0;
+	int i;
 
-	s = strchr(matcher, '(');
+	name_b = matcher;
+	if (matcher != NULL) {
+		s = strchr(matcher,'(');
 
-	if (s == NULL) {
-		/* Simple match, max one hit */
-		VTAILQ_FOREACH(b, &backends, list) {
-			CHECK_OBJ_NOTNULL(b, BACKEND_MAGIC);
-			if (strcmp(b->vcl_name, matcher) == 0) {
-				if (r && found < n)
-					r[found] = b;
-				found++;
-			}
-		}
-		return found;
-	}
+		if (s != NULL)
+			name_l = s - name_b;
+		else
+			name_l = strlen(name_b);
 
-	vcl_name = strndup(matcher, s - matcher);
-	AN(vcl_name);
-	s++;
-	while (*s != ')') {
-		if (*s == ':') {
-			/* Port */
+		if (s != NULL) {
 			s++;
-			match_port = s;
-			if (!(s = strchr(match_port, ','))) {
-				s = strchr(match_port, ')');
+			while (isspace(*s))
+				s++;
+			ip_b = s;
+			while (*s != '\0' &&
+			    *s != ')' &&
+			    *s != ':' &&
+			    !isspace(*s))
+				s++;
+			ip_l = s - ip_b;
+			while (isspace(*s))
+				s++;
+			if (*s == ':') {
+				s++;
+				while (isspace(*s))
+					s++;
+				port_b = s;
+				while (*s != '\0' && *s != ')' && !isspace(*s))
+					s++;
+				port_l = s - port_b;
 			}
-			XXXAN(s);
-			match_port = strndup(match_port, s - match_port);
-			AN(match_port);
-			if (*s == ',')
+			while (isspace(*s))
 				s++;
-		} else {
-			/* IP */
-			match_ip = s;
-			if (!(s = strchr(match_ip, ','))) {
-				s = strchr(match_ip, ')');
+			if (*s != ')') {
+				VCLI_Out(cli,
+				    "Match string syntax error:"
+				    " ')' not found.");
+				VCLI_SetResult(cli, CLIS_CANT);
+				return (-1);
 			}
-			XXXAN(s);
-			match_ip = strndup(match_ip, s - match_ip);
-			AN(match_ip);
-			if (*s == ',')
+			s++;
+			while (isspace(*s))
 				s++;
+			if (*s != '\0') {
+				VCLI_Out(cli,
+				    "Match string syntax error:"
+				    " junk after ')'");
+				VCLI_SetResult(cli, CLIS_CANT);
+				return (-1);
+			}
 		}
 	}
 	VTAILQ_FOREACH(b, &backends, list) {
 		CHECK_OBJ_NOTNULL(b, BACKEND_MAGIC);
-		if (match_port && strcmp(b->port, match_port) != 0)
+		if (port_b != NULL && strncmp(b->port, port_b, port_l) != 0)
 			continue;
-		if (match_ip &&
-		    (strcmp(b->ipv4_addr, match_ip) != 0) &&
-		    (strcmp(b->ipv6_addr, match_ip) != 0))
+		if (name_b != NULL && strncmp(b->vcl_name, name_b, name_l) != 0)
 			continue;
-		if (strcmp(b->vcl_name, vcl_name) == 0) {
-			if (r && found < n)
-				r[found] = b;
-			found++;
-		}
+		if (ip_b != NULL &&
+		    (b->ipv4_addr == NULL ||
+		      strncmp(b->ipv4_addr, ip_b, ip_l)) &&
+		    (b->ipv6_addr == NULL ||
+		      strncmp(b->ipv6_addr, ip_b, ip_l)))
+			continue;
+		found++;
+		i = func(cli, b, priv);
+		if (i)
+			return(i);
+	}
+	return (found);
+}
+
+/*---------------------------------------------------------------------*/
+
+static int __match_proto__()
+do_list(struct cli *cli, struct backend *b, void *priv)
+{
+	int *hdr;
+	char buf[128];
+
+	AN(priv);
+	hdr = priv;
+	if (!*hdr) {
+		VCLI_Out(cli, "%-30s %-6s %-10s %s",
+		    "Backend name", "Refs", "Admin", "Probe");
+		*hdr = 1;
+	}
+	CHECK_OBJ_NOTNULL(b, BACKEND_MAGIC);
+
+	bprintf(buf, "%s(%s,%s,%s)",
+		b->vcl_name,
+		b->ipv4_addr == NULL ? "" : b->ipv4_addr,
+		b->ipv6_addr == NULL ? "" : b->ipv6_addr, b->port);
+	VCLI_Out(cli, "\n%-30s %-6d", buf, b->refcount);
+
+	if (b->admin_health == ah_probe)
+		VCLI_Out(cli, " %-10s", "probe");
+	else if (b->admin_health == ah_sick)
+		VCLI_Out(cli, " %-10s", "sick");
+	else if (b->admin_health == ah_healthy)
+		VCLI_Out(cli, " %-10s", "healthy");
+	else
+		VCLI_Out(cli, " %-10s", "invalid");
+
+	if (b->probe == NULL)
+		VCLI_Out(cli, " %s", "Healthy (no probe)");
+	else {
+		if (b->healthy)
+			VCLI_Out(cli, " %s", "Healthy ");
+		else
+			VCLI_Out(cli, " %s", "Sick ");
+		VBP_Summary(cli, b->probe);
 	}
-	return found;
+
+	return (0);
 }
 
 static void
 cli_backend_list(struct cli *cli, const char * const *av, void *priv)
 {
-	struct backend *b;
-	const char *ah;
+	int hdr = 0;
 
 	(void)av;
 	(void)priv;
 	ASSERT_CLI();
-	VCLI_Out(cli, "%-30s %10s %15s %15s", "Backend name",
-		 "Conns", "Probed healthy", "Admin health");
-	VTAILQ_FOREACH(b, &backends, list) {
-		char buf[128];
-		CHECK_OBJ_NOTNULL(b, BACKEND_MAGIC);
+	(void)backend_find(cli, av[2], do_list, &hdr);
+}
 
-		bprintf(buf, "%s(%s,%s,%s)",
-			b->vcl_name,
-			b->ipv4_addr == NULL ? "" : b->ipv4_addr,
-			b->ipv6_addr == NULL ? "" : b->ipv6_addr, b->port);
-		if (b->admin_health == from_probe) {
-			ah = "Auto";
-		} else if (b->admin_health == sick) {
-			ah = "Sick";
-		} else {
-			ah = "Healthy";
-		}
-		VCLI_Out(cli, "%p %s(%s,%s,:%s) %d %d\n",
-		    b, b->vcl_name, b->ipv4_addr, b->ipv6_addr, b->port,
-		    b->refcount, b->n_conn);
-		VCLI_Out(cli, "\n%-30s %10d %15s %15s",
-			 buf,
-			 b->refcount,
-			 (b ->healthy ? "Yes" : "No"),
-			 ah);
-	}
+/*---------------------------------------------------------------------*/
+
+static int __match_proto__()
+do_set_health(struct cli *cli, struct backend *b, void *priv)
+{
+	enum admin_health state;
+
+	(void)cli;
+	state = *(enum admin_health*)priv;
+	CHECK_OBJ_NOTNULL(b, BACKEND_MAGIC);
+	b->admin_health = state;
+	return (0);
 }
 
 static void
 cli_backend_set_health(struct cli *cli, const char * const *av, void *priv)
 {
-	struct backend **b;
-	enum health_status state;
+	enum admin_health state;
 	int n;
-	const char *wstate;
 
 	(void)av;
 	(void)priv;
 	ASSERT_CLI();
-	wstate = av[3];
-	if (strcmp(wstate, "healthy") == 0) {
-		state = healthy;
-	} else if (strcmp(wstate, "sick") == 0) {
-		state = sick;
-	} else if (strcmp(wstate, "auto") == 0) {
-		state = from_probe;
-	} else {
-		VCLI_Out(cli, "Invalid state %s", wstate);
-		VCLI_SetResult(cli, CLIS_CANT);
+	AN(av[2]);
+	AN(av[3]);
+	state = vbe_str2adminhealth(av[3]);
+	if (state == ah_invalid) {
+		VCLI_Out(cli, "Invalid state %s", av[3]);
+		VCLI_SetResult(cli, CLIS_PARAM);
 		return;
 	}
-	n = backend_find(av[2], NULL, 0);
+	n = backend_find(cli, av[2], do_set_health, &state);
 	if (n == 0) {
-		VCLI_Out(cli, "No matching backends");
-		VCLI_SetResult(cli, CLIS_CANT);
-		return;
-	}
-
-	b = calloc(n, sizeof(struct backend *));
-	AN(b);
-	n = backend_find(av[2], b, n);
-
-	VCLI_Out(cli, "Set state to %s for the following backends:", wstate);
-	for (int i = 0; i < n; i++) {
-		char buf[128];
-		bprintf(buf, "%s(%s,%s,%s)",
-			b[i]->vcl_name,
-			b[i]->ipv4_addr == NULL ? "" : b[i]->ipv4_addr,
-			b[i]->ipv6_addr == NULL ? "" : b[i]->ipv6_addr, b[i]->port);
-
-		b[i]->admin_health = state;
-		VCLI_Out(cli, "\n\t%s", buf);
+		VCLI_Out(cli, "No Backends matches");
+		VCLI_SetResult(cli, CLIS_PARAM);
 	}
 }
 
+/*---------------------------------------------------------------------*/
+
 static struct cli_proto backend_cmds[] = {
 	{ "backend.list", "backend.list",
-	    "\tList all backends\n", 0, 0, "d", cli_backend_list },
+	    "\tList all backends\n", 0, 1, "d", cli_backend_list },
 	{ "backend.set_health", "backend.set_health matcher state",
 	    "\tShow a backend\n", 2, 2, "d", cli_backend_set_health },
 	{ NULL }
 };
 
-/*--------------------------------------------------------------------*/
+/*---------------------------------------------------------------------*/
 
 void
 VBE_Init(void)

bin/varnishd/cache_backend_poll.c

@@ -362,6 +362,14 @@ vbp_wrk_poll_backend(void *priv)
  * Cli functions
  */
 
+void
+VBP_Summary(struct cli *cli, const struct vbp_target *vt)
+{
+
+	CHECK_OBJ_NOTNULL(vt, VBP_TARGET_MAGIC);
+	VCLI_Out(cli, "%d/%d", vt->good, vt->probe.window);
+}
+
 static void
 vbp_bitmap(struct cli *cli, char c, uint64_t map, const char *lbl)
 {

bin/varnishtest/tests/c00048.vtc

@@ -49,7 +49,7 @@ client c1 {
 	expect resp.status == 200
 } -run
 
-varnish v1 -clierr 300 "backend.set_health s1 foo"
-varnish v1 -clierr 300 "backend.set_health s2 foo"
-varnish v1 -clierr 300 "backend.set_health s2 auto"
+varnish v1 -clierr 106 "backend.set_health s1 foo"
+varnish v1 -clierr 106 "backend.set_health s2 foo"
+varnish v1 -clierr 106 "backend.set_health s2 auto"