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

[Dridi Boukelmoune]

On Wed, Jan 20, 2021 at 2:39 PM Nils Goroll <nils.goroll at uplex.de> wrote:
>
>
> 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);");

Can we at least format the generated code to be readable?

Break line and indent?

>         /* Emit method functions */
>         Fh(tl, 1, "\n");
> _______________________________________________
> varnish-commit mailing list
> varnish-commit at varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit