[3.0] 1259255 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 ...
Tollef Fog Heen
tfheen at varnish-cache.org
Mon Apr 16 10:20:35 CEST 2012
commit 1259255959e01744b7c3e93b711fbb0a1a9e909f
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Mon Nov 7 11:06:42 2011 +0000
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.
diff --git a/bin/varnishd/cache_backend.c b/bin/varnishd/cache_backend.c
index b296761..756d659 100644
--- a/bin/varnishd/cache_backend.c
+++ b/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 */
diff --git a/bin/varnishd/cache_backend.h b/bin/varnishd/cache_backend.h
index 25148ac..059b4e7 100644
--- a/bin/varnishd/cache_backend.h
+++ b/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*);
diff --git a/bin/varnishd/cache_backend_cfg.c b/bin/varnishd/cache_backend_cfg.c
index 215a4dc..955ae26 100644
--- a/bin/varnishd/cache_backend_cfg.c
+++ b/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)
diff --git a/bin/varnishd/cache_backend_poll.c b/bin/varnishd/cache_backend_poll.c
index 0854dfd..ce00a67 100644
--- a/bin/varnishd/cache_backend_poll.c
+++ b/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)
{
diff --git a/bin/varnishtest/tests/c00048.vtc b/bin/varnishtest/tests/c00048.vtc
index ed8671f..a4d0847 100644
--- a/bin/varnishtest/tests/c00048.vtc
+++ b/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"
More information about the varnish-commit
mailing list