[master] 5735a543c Insist that VCL_RET_FAIL goes through VRT_fail() and not VRT_handling()

[Dridi Boukelmoune]

On Tue, Feb 12, 2019 at 11:02 AM Poul-Henning Kamp <phk at freebsd.org> wrote:
>
>
> commit 5735a543c720655c484ef11c70b6cde7f201d5ca
> Author: Poul-Henning Kamp <phk at FreeBSD.org>
> Date:   Tue Feb 12 10:01:00 2019 +0000
>
>     Insist that VCL_RET_FAIL goes through VRT_fail() and not VRT_handling()

This is a hard VRT breakage, if it's not documented in vrt.h, someone
please fix that.

This will obviously not land in 6.0 when back-ports reach this point.

Dridi

> diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
> index 45b4e1399..8940a6797 100644
> --- a/bin/varnishd/cache/cache_vrt.c
> +++ b/bin/varnishd/cache/cache_vrt.c
> @@ -491,11 +491,11 @@ VRT_handling(VRT_CTX, unsigned hand)
>  {
>
>         CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
> -       if (ctx->handling == NULL)
> -               return;
> +       assert(hand != VCL_RET_FAIL);
> +       AN(ctx->handling);
> +       AZ(*ctx->handling);
>         assert(hand > 0);
>         assert(hand < VCL_RET_MAX);
> -       // XXX:NOTYET assert(*ctx->handling == 0);
>         *ctx->handling = hand;
>  }
>
> @@ -507,16 +507,21 @@ VRT_fail(VRT_CTX, const char *fmt, ...)
>         va_list ap;
>
>         assert(ctx->vsl != NULL || ctx->msg != NULL);
> +       AN(ctx->handling);
> +       if (*ctx->handling == VCL_RET_FAIL)
> +               return;
> +       AZ(*ctx->handling);
> +       AN(fmt);
>         AZ(strchr(fmt, '\n'));
>         va_start(ap, fmt);
> -       if (ctx->vsl != NULL)
> +       if (ctx->vsl != NULL) {
>                 VSLbv(ctx->vsl, SLT_VCL_Error, fmt, ap);
> -       else {
> +       } else {
>                 VSB_vprintf(ctx->msg, fmt, ap);
>                 VSB_putc(ctx->msg, '\n');
>         }
>         va_end(ap);
> -       VRT_handling(ctx, VCL_RET_FAIL);
> +       *ctx->handling = VCL_RET_FAIL;
>  }
>
>  /*--------------------------------------------------------------------
> @@ -772,9 +777,8 @@ VRT_purge(VRT_CTX, VCL_DURATION ttl, VCL_DURATION grace, VCL_DURATION keep)
>         CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
>
>         if ((ctx->method & (VCL_MET_HIT|VCL_MET_MISS)) == 0) {
> -               VSLb(ctx->vsl, SLT_VCL_Error,
> +               VRT_fail(ctx,
>                     "purge can only happen in vcl_hit{} or vcl_miss{}");
> -               VRT_handling(ctx, VCL_RET_FAIL);
>                 return (0);
>         }
>
> diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c
> index ded906870..02d323a14 100644
> --- a/lib/libvcc/vcc_action.c
> +++ b/lib/libvcc/vcc_action.c
> @@ -350,7 +350,10 @@ vcc_act_return(struct vcc *tl, struct token *t, struct symbol *sym)
>                 }
>         }
>         ERRCHK(tl);
> -       Fb(tl, 1, "VRT_handling(ctx, VCL_RET_%s);\n", h);
> +       if (hand == VCL_RET_FAIL)
> +               Fb(tl, 1, "VRT_fail(ctx, \"Failed from VCL\");\n");
> +       else
> +               Fb(tl, 1, "VRT_handling(ctx, VCL_RET_%s);\n", h);
>         SkipToken(tl, ')');
>         SkipToken(tl, ';');
>  }
> diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
> index 13994eb6b..203f34465 100644
> --- a/lib/libvcc/vcc_compile.c
> +++ b/lib/libvcc/vcc_compile.c
> @@ -318,8 +318,15 @@ EmitInitFini(const struct vcc *tl)
>                 if (VSB_len(p->event))
>                         has_event = 1;
>         }
> +
> +       /* Handle failures from vcl_init */
> +       Fc(tl, 0, "\n");
> +       Fc(tl, 0, "\tif (*ctx->handling != VCL_RET_OK)\n");
> +       Fc(tl, 0, "\t\treturn(1);\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");
> @@ -655,7 +662,12 @@ vcc_CompileSource(struct vcc *tl, struct source *sp)
>
>         /* Tie vcl_init/fini in */
>         ifp = New_IniFin(tl);
> -       VSB_printf(ifp->ini, "\tVGC_function_vcl_init(ctx);");
> +       VSB_printf(ifp->ini, "\tVGC_function_vcl_init(ctx);\n");
> +       /*
> +        * We do not return(1) if this fails, because the failure
> +        * could be half way into vcl_init{} so vcl_fini{} must
> +        * always be called, also on failure.
> +        */
>         VSB_printf(ifp->fin, "\t\tVGC_function_vcl_fini(ctx);");
>
>         /* Emit method functions */
> _______________________________________________
> varnish-commit mailing list
> varnish-commit at varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit