[master] 4ccd356d1 Clarify (struct vrt_ctx).handling for PRIVs

[Nils Goroll]

commit 4ccd356d1bdcd49af4c76399c492c3d278940324
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Wed Jan 20 14:16:11 2021 +0100

    Clarify (struct vrt_ctx).handling for PRIVs
    
    Ref 43d9e5fb1a10a88ab6a5a98ad4038438025c4999 :
    
    PRIV_* fini methods need to leave (struct vrt_ctx).handling alone,
    except that they might call VRT_fail(), see also
    746384b20cbc24ff8afd2df35e1510087404fbf4
    
    Thus we add assertions that handling be either 0 or VCL_RET_FAIL
    outside the FSM.
    
    To be able to do so, we need to change VCL_RET_OK into 0 when
    vcl_init{} has returned successfully.
    
    The vcl_fini{} case is slightly more complicated:
    
    By design, only "ok" (VCL_RET_OK) is allowed, but VRT_fail() also
    added VCL_RET_FAIL, so we de-facto get a "fail" return if any vmod
    code called VRT_fail().
    
    Because PRIV_* handling happens from VCC generated code via
    VGC_Discard(), we need to check and change (struct vrt_ctx).handling
    right after calling vcl_fini{} / VGC_function_vcl_fini() from
    VGC_Discard(). This is VPI_vcl_fini().
    
    Implementation note:
    
    I also considered void VPI_vcl_fini(VRT_CTX, vcl_func_f fini_sub),
    having VPI_vcl_fini call the fini_sub, but that stirred up includes of
    VPI where vcl.h is not included.

diff --git a/bin/varnishd/cache/cache_vpi.c b/bin/varnishd/cache/cache_vpi.c
index 3b6783ba8..06eb802d5 100644
--- a/bin/varnishd/cache/cache_vpi.c
+++ b/bin/varnishd/cache/cache_vpi.c
@@ -62,6 +62,26 @@ VPI_count(VRT_CTX, unsigned u)
 		    ctx->vcl->conf->ref[u].line, ctx->vcl->conf->ref[u].pos);
 }
 
+/*
+ * After vcl_fini {} == VGC_function_vcl_fini() is called from VGC_Discard(),
+ * handling must either be OK from VCL "return (ok)" or FAIL from VRT_fail().
+ *
+ * replace OK with 0 for _fini callbacks because that handling has meaning only
+ * when returning from VCL subs
+ */
+
+void
+VPI_vcl_fini(VRT_CTX)
+{
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	AN(ctx->handling);
+
+	if (*ctx->handling == VCL_RET_FAIL)
+		return;
+	assert(*ctx->handling == VCL_RET_OK);
+	*ctx->handling = 0;
+}
+
 VCL_VCL
 VPI_vcl_get(VRT_CTX, const char *name)
 {
diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
index b265e061b..40434dc6e 100644
--- a/bin/varnishd/cache/cache_vrt_priv.c
+++ b/bin/varnishd/cache/cache_vrt_priv.c
@@ -277,6 +277,7 @@ VRT_priv_fini(VRT_CTX, const struct vmod_priv *p)
 	VRT_CTX_Assert(ctx);
 
 	m->fini(ctx, p->priv);
+	assert(*ctx->handling == 0 || *ctx->handling == VCL_RET_FAIL);
 }
 
 /*--------------------------------------------------------------------*/
@@ -293,6 +294,10 @@ VCL_TaskLeave(VRT_CTX, struct vrt_privs *privs)
 {
 	struct vrt_priv *vp, *vp1;
 
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	AN(ctx->handling);
+	assert(*ctx->handling == 0 || *ctx->handling == VCL_RET_FAIL);
+
 	/*
 	 * NB: We don't bother removing entries as we finish them because it's
 	 * a costly operation. Instead we safely walk the whole tree and clear
diff --git a/include/vcc_interface.h b/include/vcc_interface.h
index 1e5f9c3cd..865c8504e 100644
--- a/include/vcc_interface.h
+++ b/include/vcc_interface.h
@@ -52,6 +52,7 @@ struct vpi_ref {
 };
 
 void VPI_count(VRT_CTX, unsigned);
+void VPI_vcl_fini(VRT_CTX);
 
 int VPI_Vmod_Init(VRT_CTX, struct vmod **hdl, unsigned nbr, void *ptr, int len,
     const char *nm, const char *path, const char *file_id, const char *backup);
diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
index 6deaff9e4..a7b2a690a 100644
--- a/lib/libvcc/vcc_compile.c
+++ b/lib/libvcc/vcc_compile.c
@@ -364,10 +364,10 @@ EmitInitFini(const struct vcc *tl)
 	Fc(tl, 0, "\n");
 	Fc(tl, 0, "\tif (*ctx->handling != VCL_RET_OK)\n");
 	Fc(tl, 0, "\t\treturn(1);\n");
+	Fc(tl, 0, "\t*ctx->handling = 0;\n");
 
 	VTAILQ_FOREACH(sy, &tl->sym_objects, sideways) {
 		Fc(tl, 0, "\tif (!%s) {\n", sy->rname);
-		Fc(tl, 0, "\t\t*ctx->handling = 0;\n");
 		Fc(tl, 0, "\t\tVRT_fail(ctx, "
 		    "\"Object %s not initialized\");\n" , sy->name);
 		Fc(tl, 0, "\t\treturn(1);\n");
@@ -741,7 +741,7 @@ vcc_CompileSource(struct vcc *tl, struct source *sp, const char *jfile)
 	 * must always be called, also on failure.
 	 */
 	ifp->ignore_errors = 1;
-	VSB_cat(ifp->fin, "\t\tVGC_function_vcl_fini(ctx);");
+	VSB_cat(ifp->fin, "\t\tVGC_function_vcl_fini(ctx);VPI_vcl_fini(ctx);");
 
 	/* Emit method functions */
 	Fh(tl, 1, "\n");