Make the trouble-list hold a uintptr_t of the objhead, instead of the

actual objhead, to make it painfully obvious, that no dereference is
allowed.

Various strokes with the polishing cloth along the way.



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

bin/varnishd/cache.h

@@ -456,7 +456,8 @@ extern pthread_t VCA_thread;
 /* cache_backend.c */
 
 struct vbe_conn *VBE_GetFd(const struct director *, struct sess *sp);
-int VBE_Healthy(const struct director *, const struct sess *sp);
+int VBE_Healthy(double now, const struct director *, uintptr_t target);
+int VBE_Healthy_sp(const struct sess *sp, const struct director *);
 void VBE_ClosedFd(struct sess *sp);
 void VBE_RecycleFd(struct sess *sp);
 void VBE_AddHostHeader(const struct sess *sp);

bin/varnishd/cache_backend.c

@@ -243,18 +243,18 @@ vbe_NewConn(void)
  */
 
 static unsigned int
-vbe_Healthy(const struct sess *sp, struct backend *backend)
+vbe_Healthy(double now, uintptr_t target, struct backend *backend)
 {
 	struct trouble *tr;
 	struct trouble *tr2;
-	struct trouble *old = NULL;
-	unsigned i = 0;
+	struct trouble *old;
+	unsigned i = 0, retval;
 	unsigned int threshold;
-	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
+
 	CHECK_OBJ_NOTNULL(backend, BACKEND_MAGIC);
 
 	if (!backend->healthy)
-		return 0;
+		return (0);
 
 	/* VRT/VCC sets threshold to UINT_MAX to mark that it's not
 	 * specified by VCL (thus use param).
@@ -266,43 +266,47 @@ vbe_Healthy(const struct sess *sp, struct backend *backend)
 
 	/* Saintmode is disabled */
 	if (threshold == 0)
-		return 1;
+		return (1);
 
 	/* No need to test if we don't have an object head to test against.
 	 * FIXME: Should check the magic too, but probably not assert?
 	 */
-	if (!sp->objhead)
-		return 1;
+	if (target == 0)
+		return (1);
 
+	old = NULL;
+	retval = 1;
 	Lck_Lock(&backend->mtx);
 	VTAILQ_FOREACH_SAFE(tr, &backend->troublelist, list, tr2) {
 		CHECK_OBJ_NOTNULL(tr, TROUBLE_MAGIC);
-		if (tr->timeout < sp->t_req) {
+
+		if (tr->timeout < now) {
 			VTAILQ_REMOVE(&backend->troublelist, tr, list);
 			old = tr;
+			retval = 1;
 			break;
 		}
 
-		if (tr->objhead == sp->objhead) {
-			Lck_Unlock(&backend->mtx);
-			return 0;
+		if (tr->target == target) {
+			retval = 0;
+			break;
 		}
 
 		/* If the threshold is at 1, a single entry on the list
 		 * will disable the backend. Since 0 is disable, ++i
 		 * instead of i++ to allow this behavior.
 		 */
-		if (++i >=threshold) {
-			Lck_Unlock(&backend->mtx);
-			return 0;
+		if (++i >= threshold) {
+			retval = 0;
+			break;
 		}
 	}
 	Lck_Unlock(&backend->mtx);
 
-	if (old)
+	if (old != NULL)
 		FREE_OBJ(old);
 
-	return 1;
+	return (retval);
 }
 
 /*--------------------------------------------------------------------
@@ -342,7 +346,7 @@ vbe_GetVbe(struct sess *sp, struct backend *bp)
 		VBE_ClosedFd(sp);
 	}
 
-	if (!vbe_Healthy(sp, bp)) {
+	if (!vbe_Healthy(sp->t_req, (uintptr_t)sp->objhead, bp)) {
 		VSL_stats->backend_unhealthy++;
 		return (NULL);
 	}
@@ -423,17 +427,28 @@ VBE_GetFd(const struct director *d, struct sess *sp)
 	return (d->getfd(d, sp));
 }
 
-/* Check health ------------------------------------------------------*/
+/* Check health ------------------------------------------------------
+ *
+ * The target is really an objhead pointer, but since it can not be
+ * dereferenced during health-checks, we pass it as uintptr_t, which
+ * hopefully will make people investigate, before mucking about with it.
+ */
 
 int
-VBE_Healthy(const struct director *d, const struct sess *sp)
+VBE_Healthy_sp(const struct sess *sp, const struct director *d)
 {
 
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
-	if (d == NULL)
-		d = sp->director;
 	CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
-	return (d->healthy(d, sp));
+	return (d->healthy(sp->t_req, d, (uintptr_t)sp->objhead));
+}
+
+int
+VBE_Healthy(double now, const struct director *d, uintptr_t target)
+{
+
+	CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
+	return (d->healthy(now, d, target));
 }
 
 /*--------------------------------------------------------------------
@@ -469,14 +484,13 @@ vdi_simple_getfd(const struct director *d, struct sess *sp)
 }
 
 static unsigned
-vdi_simple_healthy(const struct director *d, const struct sess *sp)
+vdi_simple_healthy(double now, const struct director *d, uintptr_t target)
 {
 	struct vdi_simple *vs;
 
-	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
 	CAST_OBJ_NOTNULL(vs, d->priv, VDI_SIMPLE_MAGIC);
-	return (vbe_Healthy(sp, vs->backend));
+	return (vbe_Healthy(now, target, vs->backend));
 }
 
 /*lint -e{818} not const-able */

bin/varnishd/cache_backend.h

@@ -79,7 +79,8 @@ struct vrt_backend_probe;
 
 typedef struct vbe_conn *vdi_getfd_f(const struct director *, struct sess *sp);
 typedef void vdi_fini_f(struct director *);
-typedef unsigned vdi_healthy(const struct director *, const struct sess *sp);
+typedef unsigned vdi_healthy(double now, const struct director *,
+    uintptr_t target);
 
 struct director {
 	unsigned		magic;
@@ -99,7 +100,7 @@ struct director {
 struct trouble {
 	unsigned		magic;
 #define TROUBLE_MAGIC		0x4211ab21
-	void			*objhead; /* NB: only comparison */
+	uintptr_t		target;
 	double			timeout;
 	VTAILQ_ENTRY(trouble)	list;
 };

bin/varnishd/cache_dir_random.c

@@ -137,7 +137,7 @@ vdi_random_getfd(const struct director *d, struct sess *sp)
 			if (r >= s1)
 				continue;
 			d2 = vs->hosts[i].backend;
-			if (!VBE_Healthy(d2, sp))
+			if (!VBE_Healthy_sp(sp, d2))
 				break;
 			vbe = VBE_GetFd(d2, sp);
 			if (vbe != NULL)
@@ -152,7 +152,7 @@ vdi_random_getfd(const struct director *d, struct sess *sp)
 		for (i = 0; i < vs->nhosts; i++) {
 			d2 = vs->hosts[i].backend;
 			/* XXX: cache result of healty to avoid double work */
-			if (VBE_Healthy(d2, sp))
+			if (VBE_Healthy_sp(sp, d2))
 				s1 += vs->hosts[i].weight;
 		}
 
@@ -171,7 +171,7 @@ vdi_random_getfd(const struct director *d, struct sess *sp)
 		s1 = 0.0;
 		for (i = 0; i < vs->nhosts; i++)  {
 			d2 = vs->hosts[i].backend;
-			if (!VBE_Healthy(d2, sp))
+			if (!VBE_Healthy_sp(sp, d2))
 				continue;
 			s1 += vs->hosts[i].weight;
 			if (r >= s1)
@@ -187,17 +187,16 @@ vdi_random_getfd(const struct director *d, struct sess *sp)
 }
 
 static unsigned
-vdi_random_healthy(const struct director *d, const struct sess *sp)
+vdi_random_healthy(double now, const struct director *d, uintptr_t target)
 {
 	struct vdi_random *vs;
 	int i;
 
-	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
 	CAST_OBJ_NOTNULL(vs, d->priv, VDI_RANDOM_MAGIC);
 
 	for (i = 0; i < vs->nhosts; i++) {
-		if (VBE_Healthy(vs->hosts[i].backend, sp))
+		if (VBE_Healthy(now, vs->hosts[i].backend, target))
 			return 1;
 	}
 	return 0;

bin/varnishd/cache_dir_round_robin.c

@@ -75,7 +75,7 @@ vdi_round_robin_getfd(const struct director *d, struct sess *sp)
 	for (i = 0; i < vs->nhosts; i++) {
 		backend = vs->hosts[vs->next_host].backend;
 		vs->next_host = (vs->next_host + 1) % vs->nhosts;
-		if (!VBE_Healthy(backend, sp))
+		if (!VBE_Healthy_sp(sp, backend))
 			continue;
 		vbe = VBE_GetFd(backend, sp);
 		if (vbe != NULL)
@@ -86,19 +86,18 @@ vdi_round_robin_getfd(const struct director *d, struct sess *sp)
 }
 
 static unsigned
-vdi_round_robin_healthy(const struct director *d, const struct sess *sp)
+vdi_round_robin_healthy(double now, const struct director *d, uintptr_t target)
 {
 	struct vdi_round_robin *vs;
 	struct director *backend;
 	int i;
 
-	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
 	CAST_OBJ_NOTNULL(vs, d->priv, VDI_ROUND_ROBIN_MAGIC);
 
 	for (i = 0; i < vs->nhosts; i++) {
 		backend = vs->hosts[i].backend;
-		if (VBE_Healthy(backend, sp))
+		if (VBE_Healthy(now, backend, target))
 			return 1;
 	}
 	return 0;

bin/varnishd/cache_hash.c

@@ -187,7 +187,7 @@ HSH_AddString(const struct sess *sp, const char *str)
 		WSP(sp, SLT_Hash, "%s", str);
 }
 
-/**********************************************************************
+/*---------------------------------------------------------------------
  * This is a debugging hack to enable testing of boundary conditions
  * in the hash algorithm.
  * We trap the first 9 different digests and translate them to different
@@ -394,15 +394,16 @@ HSH_Lookup(struct sess *sp, struct objhead **poh)
 	 * mode'. Is this entirely wrong, or just ugly? Why isn't objhead
 	 * set here? FIXME:Grace.
 	 */
-	sp->objhead = oh;
-	if (oc == NULL && grace_oc != NULL &&
-	    (busy_oc != NULL || !VBE_Healthy(NULL, sp))) {
+	if (oc == NULL			/* We found no live object */
+	    && grace_oc != NULL		/* There is a grace candidate */
+	    && (busy_oc != NULL		/* Somebody else is already busy */
+	    || !VBE_Healthy(sp->t_req, sp->director, (uintptr_t)oh))) {
+					 /* Or it is impossible to fetch: */
 		o = grace_oc->obj;
 		CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
 		if (o->ttl + HSH_Grace(sp->grace) >= sp->t_req)
 			oc = grace_oc;
 	}
-	sp->objhead = NULL;
 
 	if (oc != NULL) {
 		o = oc->obj;

bin/varnishd/cache_vrt.c

@@ -308,7 +308,7 @@ VRT_l_beresp_saintmode(const struct sess *sp, double a)
 
 	ALLOC_OBJ(new, TROUBLE_MAGIC);
 	AN(new);
-	new->objhead = sp->objhead;
+	new->target = (uintptr_t)sp->objhead;
 	new->timeout = sp->t_req + a;
 
 	/* Insert the new item on the list before the first item with a
@@ -782,7 +782,7 @@ VRT_r_req_backend_healthy(const struct sess *sp)
 {
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(sp->director, DIRECTOR_MAGIC);
-	return (VBE_Healthy(NULL, sp));
+	return (VBE_Healthy_sp(sp, sp->director));
 }
 
 /*--------------------------------------------------------------------*/