[master] 00fa3c754 Restore previous VRT_AddDirector semantics

Dridi Boukelmoune dridi.boukelmoune at gmail.com
Fri Dec 27 09:25:05 UTC 2019


commit 00fa3c754a2abe04c55fa9825acfb1a2d3c800e0
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Fri Dec 27 10:12:56 2019 +0100

    Restore previous VRT_AddDirector semantics
    
    Upon failure, don't destroy the underlying director implementation. When
    the deletion code was split in c671bb6691f27172ae9103226cb09a99c9812483
    in order to be reused in 615298547adfb06aa785b42552062bc391f56ecd, it
    mistakenly included the director implementation.
    
    Instead, it's really the caller's job to tear down the backend if it
    couldn't be added. Since vbe_destroy operates on a director, it is split
    in two to avoid reintroducing the previous "undo" code duplication.
    
    Refs #3176

diff --git a/bin/varnishd/cache/cache_backend.c b/bin/varnishd/cache/cache_backend.c
index 84f42b21a..d1c4b90b7 100644
--- a/bin/varnishd/cache/cache_backend.c
+++ b/bin/varnishd/cache/cache_backend.c
@@ -391,12 +391,11 @@ vbe_dir_event(const struct director *d, enum vcl_event_e ev)
 
 /*---------------------------------------------------------------------*/
 
-static void v_matchproto_(vdi_destroy_f)
-vbe_destroy(const struct director *d)
+static void
+vbe_free(struct backend *be)
 {
-	struct backend *be;
 
-	CAST_OBJ_NOTNULL(be, d->priv, BACKEND_MAGIC);
+	CHECK_OBJ_NOTNULL(be, BACKEND_MAGIC);
 
 	if (be->probe != NULL)
 		VBP_Remove(be);
@@ -421,6 +420,15 @@ vbe_destroy(const struct director *d)
 	FREE_OBJ(be);
 }
 
+static void v_matchproto_(vdi_destroy_f)
+vbe_destroy(const struct director *d)
+{
+	struct backend *be;
+
+	CAST_OBJ_NOTNULL(be, d->priv, BACKEND_MAGIC);
+	vbe_free(be);
+}
+
 /*--------------------------------------------------------------------*/
 
 static void
@@ -550,7 +558,6 @@ VCL_BACKEND
 VRT_new_backend_clustered(VRT_CTX, struct vsmw_cluster *vc,
     const struct vrt_backend *vrt)
 {
-	VCL_BACKEND d;
 	struct backend *be;
 	struct vcl *vcl;
 	const struct vrt_backend_probe *vbp;
@@ -607,18 +614,17 @@ VRT_new_backend_clustered(VRT_CTX, struct vsmw_cluster *vc,
 	VSC_C_main->n_backend++;
 	Lck_Unlock(&backends_mtx);
 
-	d = VRT_AddDirector(ctx, m, be, "%s", vrt->vcl_name);
-
-	/* NB: if VRT_AddDirector failed, be was already freed. */
-	if (d != NULL) {
-		be->director = d;
+	be->director = VRT_AddDirector(ctx, m, be, "%s", vrt->vcl_name);
 
+	if (be->director != NULL) {
 		/* for cold VCL, update initial director state */
 		if (be->probe != NULL && ! vcl->temp->is_warm)
 			VBP_Update_Backend(be->probe);
+		return (be->director);
 	}
 
-	return (d);
+	vbe_free(be);
+	return (NULL);
 }
 
 VCL_BACKEND
diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c
index 2c63b4b1a..228517d5c 100644
--- a/bin/varnishd/cache/cache_vrt_vcl.c
+++ b/bin/varnishd/cache/cache_vrt_vcl.c
@@ -41,8 +41,6 @@
 #include "cache_director.h"
 #include "cache_vcl.h"
 
-static void deldirector(struct vcldir *);
-
 /*--------------------------------------------------------------------*/
 
 const char *
@@ -142,6 +140,15 @@ VCL_Rel(struct vcl **vcc)
 
 /*--------------------------------------------------------------------*/
 
+static void
+vcldir_free(struct vcldir *vdir)
+{
+
+	free(vdir->cli_name);
+	FREE_OBJ(vdir->dir);
+	FREE_OBJ(vdir);
+}
+
 VCL_BACKEND
 VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
     const char *fmt, ...)
@@ -159,6 +166,10 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
 	vcl = ctx->vcl;
 	CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
 
+	// opportunistic, re-checked again under lock
+	if (vcl->temp == VCL_TEMP_COOLING && !DO_DEBUG(DBG_VTC_MODE))
+		return (NULL);
+
 	ALLOC_OBJ(vdir, VCLDIR_MAGIC);
 	AN(vdir);
 	ALLOC_OBJ(vdir->dir, DIRECTOR_MAGIC);
@@ -198,7 +209,7 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
 	Lck_Unlock(&vcl_mtx);
 
 	if (temp == VCL_TEMP_COOLING) {
-		deldirector(vdir);
+		vcldir_free(vdir);
 		return (NULL);
 	}
 	if (!temp->is_warm && temp != VCL_TEMP_INIT)
@@ -207,16 +218,6 @@ VRT_AddDirector(VRT_CTX, const struct vdi_methods *m, void *priv,
 	return (vdir->dir);
 }
 
-static void
-deldirector(struct vcldir *vdir)
-{
-	if(vdir->methods->destroy != NULL)
-		vdir->methods->destroy(vdir->dir);
-	free(vdir->cli_name);
-	FREE_OBJ(vdir->dir);
-	FREE_OBJ(vdir);
-}
-
 void
 VRT_DelDirector(VCL_BACKEND *bp)
 {
@@ -238,8 +239,10 @@ VRT_DelDirector(VCL_BACKEND *bp)
 
 	if (temp->is_warm)
 		VDI_Event(d, VCL_EVENT_COLD);
+	if(vdir->methods->destroy != NULL)
+		vdir->methods->destroy(d);
 	assert (d == vdir->dir);
-	deldirector(vdir);
+	vcldir_free(vdir);
 }
 
 void


More information about the varnish-commit mailing list