On Thu, Dec 10, 2015 at 4:30 PM, Dridi Boukelmoune <[email protected]> wrote: >> Hi, >> >> Here is a new batch of patches fix changes requested from a first >> review. There's one more commit this time, it's number 3. > > One more update for the first review, I forgot to update patch number 1.
Hi, Same patches as last time, rebased against current master. I sneaked in a trivial patch (0001) taking advantage of the review. Cheers, Dridi
From ddb4bf18d32562817b97c0488f3ff532f854d7c3 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Tue, 5 Jan 2016 15:00:03 +0100 Subject: [PATCH 1/9] Display the WRONG() message in VAS_Fail_default --- lib/libvarnish/vas.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/libvarnish/vas.c b/lib/libvarnish/vas.c index 2d810d2..0c857a0 100644 --- a/lib/libvarnish/vas.c +++ b/lib/libvarnish/vas.c @@ -1,6 +1,6 @@ /*- * Copyright (c) 2006 Verdens Gang AS - * Copyright (c) 2006-2011 Varnish Software AS + * Copyright (c) 2006-2016 Varnish Software AS * All rights reserved. * * Author: Poul-Henning Kamp <[email protected]> @@ -46,7 +46,7 @@ VAS_Fail_default(const char *func, const char *file, int line, if (kind == VAS_MISSING) { fprintf(stderr, - "Missing errorhandling code in %s(), %s line %d:\n" + "Missing error handling code in %s(), %s line %d:\n" " Condition(%s) not true.\n", func, file, line, cond); } else if (kind == VAS_INCOMPLETE) { @@ -55,8 +55,8 @@ VAS_Fail_default(const char *func, const char *file, int line, func, file, line); } else if (kind == VAS_WRONG) { fprintf(stderr, - "Wrong turn in %s(), %s line %d:\n", - func, file, line); + "Wrong turn in %s(), %s line %d: %s\n", + func, file, line, cond); } else { fprintf(stderr, "Assert error in %s(), %s line %d:\n" -- 2.4.3
From b09f25c0d93b5d1fb8542f71df2867ad593a5d8a Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Fri, 4 Dec 2015 09:48:11 +0100 Subject: [PATCH 2/9] Turn VCL state magic numbers into an enum Make a clear distinction between (struct vclprog).warm that is used as a boolean, unlike the second parameter of mgt_vcl_setstate. We don't use an actual C enum but const char[] to make debugging easier. --- bin/varnishd/mgt/mgt_vcl.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c index 37359f2..583f478 100644 --- a/bin/varnishd/mgt/mgt_vcl.c +++ b/bin/varnishd/mgt/mgt_vcl.c @@ -48,11 +48,15 @@ #include "mgt_cli.h" +static const char * const vcl_state_cold = "cold"; +static const char * const vcl_state_warm = "warm"; +static const char * const vcl_state_auto = "auto"; + struct vclprog { VTAILQ_ENTRY(vclprog) list; char *name; char *fname; - int warm; + unsigned warm; char state[8]; double go_cold; }; @@ -120,21 +124,24 @@ mgt_has_vcl(void) } static void -mgt_vcl_setstate(struct vclprog *vp, int warm) +mgt_vcl_setstate(struct vclprog *vp, const char *vs) { - unsigned status; + unsigned status, warm; double now; char *p; - if (warm == -1) { + if (vs == vcl_state_auto) { assert(vp != active_vcl); now = VTIM_mono(); - warm = vp->warm; + vs = vp->warm ? vcl_state_warm : vcl_state_cold; if (vp->go_cold > 0 && !strcmp(vp->state, "auto") && vp->go_cold + mgt_param.vcl_cooldown < now) - warm = 0; + vs = vcl_state_cold; } + assert(vs != vcl_state_auto); + warm = vs == vcl_state_warm ? 1 : 0; + if (vp->warm == warm) return; @@ -230,7 +237,7 @@ mgt_push_vcls_and_start(unsigned *status, char **p) struct vclprog *vp; AN(active_vcl); - mgt_vcl_setstate(active_vcl, 1); + mgt_vcl_setstate(active_vcl, vcl_state_warm); VTAILQ_FOREACH(vp, &vclhead, list) { if (mgt_cli_askchild(status, p, "vcl.load \"%s\" %s %d%s\n", vp->name, vp->fname, vp->warm, vp->state)) @@ -321,7 +328,7 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv) bprintf(vp->state, "%s", "auto"); if (vp != active_vcl) { vp->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp, -1); + mgt_vcl_setstate(vp, vcl_state_auto); } } else if (!strcmp(av[3], "cold")) { if (vp == active_vcl) { @@ -330,10 +337,10 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv) return; } bprintf(vp->state, "%s", "auto"); - mgt_vcl_setstate(vp, 0); + mgt_vcl_setstate(vp, vcl_state_cold); } else if (!strcmp(av[3], "warm")) { bprintf(vp->state, "%s", av[3]); - mgt_vcl_setstate(vp, 1); + mgt_vcl_setstate(vp, vcl_state_warm); } else { VCLI_Out(cli, "State must be one of auto, cold or warm."); VCLI_SetResult(cli, CLIS_PARAM); @@ -353,20 +360,20 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv) return; if (vp == active_vcl) return; - mgt_vcl_setstate(vp, 1); + mgt_vcl_setstate(vp, vcl_state_warm); if (child_pid >= 0 && mgt_cli_askchild(&status, &p, "vcl.use %s\n", av[2])) { VCLI_SetResult(cli, status); VCLI_Out(cli, "%s", p); vp->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp, -1); + mgt_vcl_setstate(vp, vcl_state_auto); } else { VCLI_Out(cli, "VCL '%s' now active", av[2]); vp2 = active_vcl; active_vcl = vp; if (vp2 != NULL) { vp2->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp2, -1); + mgt_vcl_setstate(vp2, vcl_state_auto); } } free(p); @@ -388,7 +395,7 @@ mcf_vcl_discard(struct cli *cli, const char * const *av, void *priv) VCLI_Out(cli, "Cannot discard active VCL program\n"); return; } - mgt_vcl_setstate(vp, 0); + mgt_vcl_setstate(vp, vcl_state_cold); if (child_pid >= 0) { /* If this fails the child is crashing, figure that later */ (void)mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]); @@ -434,7 +441,7 @@ mgt_vcl_poker(const struct vev *e, int what) e_poker->timeout = mgt_param.vcl_cooldown * .45; VTAILQ_FOREACH(vp, &vclhead, list) { if (vp != active_vcl) - mgt_vcl_setstate(vp, -1); + mgt_vcl_setstate(vp, vcl_state_auto); } return (0); } -- 2.4.3
From ab2a98cd8cea728549f30702aa07effa8e45f025 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Fri, 4 Dec 2015 11:14:29 +0100 Subject: [PATCH 3/9] Make vcl_set_state accept a ctx instead of a vcl This paves the way towards failing VCL_EVENT_WARM events in VMODs. If setting the temperature fails, we may need to carry a message back to the CLI and other bits of context. --- bin/varnishd/cache/cache_vcl.c | 43 +++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index 54effd1..d26c9af 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -429,17 +429,16 @@ vcl_find(const char *name) } static void -vcl_set_state(struct vcl *vcl, const char *state) +vcl_set_state(VRT_CTX, const char *state) { - struct vrt_ctx ctx; - unsigned hand = 0; + struct vcl *vcl; ASSERT_CLI(); - AN(vcl->temp); + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + AN(ctx->handling); - INIT_OBJ(&ctx, VRT_CTX_MAGIC); - ctx.handling = &hand; - ctx.vcl = vcl; + vcl = ctx->vcl; + AN(vcl->temp); switch(state[0]) { case '0': @@ -449,7 +448,7 @@ vcl_set_state(struct vcl *vcl, const char *state) vcl->temp = vcl->refcount ? vcl_temp_cooling : vcl_temp_cold; - AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_COLD)); + AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD)); vcl_BackendEvent(vcl, VCL_EVENT_COLD); } else if (vcl->busy) @@ -466,7 +465,7 @@ vcl_set_state(struct vcl *vcl, const char *state) /* The VCL must first reach a stable cold state */ else if (vcl->temp != vcl_temp_cooling) { vcl->temp = vcl_temp_warm; - (void)vcl->conf->event_vcl(&ctx, VCL_EVENT_WARM); + (void)vcl->conf->event_vcl(ctx, VCL_EVENT_WARM); vcl_BackendEvent(vcl, VCL_EVENT_WARM); } break; @@ -529,7 +528,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) return (1); } VSB_delete(vsb); - vcl_set_state(vcl, state); + vcl_set_state(&ctx, state); bprintf(vcl->state, "%s", state + 1); assert(hand == VCL_RET_OK); VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name); @@ -577,13 +576,19 @@ VCL_Nuke(struct vcl *vcl) void VCL_Poll(void) { + struct vrt_ctx ctx; struct vcl *vcl, *vcl2; + unsigned hand; ASSERT_CLI(); VTAILQ_FOREACH_SAFE(vcl, &vcl_head, list, vcl2) { if (vcl->temp == vcl_temp_busy || - vcl->temp == vcl_temp_cooling) - vcl_set_state(vcl, "0"); + vcl->temp == vcl_temp_cooling) { + INIT_OBJ(&ctx, VRT_CTX_MAGIC); + ctx.vcl = vcl; + ctx.handling = &hand; + vcl_set_state(&ctx, "0"); + } if (vcl->discard && vcl->temp == vcl_temp_cold) VCL_Nuke(vcl); } @@ -625,17 +630,21 @@ ccf_config_load(struct cli *cli, const char * const *av, void *priv) static void __match_proto__(cli_func_t) ccf_config_state(struct cli *cli, const char * const *av, void *priv) { - struct vcl *vcl; + struct vrt_ctx ctx; + unsigned hand; + + INIT_OBJ(&ctx, VRT_CTX_MAGIC); + ctx.handling = &hand; (void)cli; AZ(priv); ASSERT_CLI(); AN(av[2]); AN(av[3]); - vcl = vcl_find(av[2]); - AN(vcl); // MGT ensures this - vcl_set_state(vcl, av[3]); - bprintf(vcl->state, "%s", av[3] + 1); + ctx.vcl = vcl_find(av[2]); + AN(ctx.vcl); // MGT ensures this + vcl_set_state(&ctx, av[3]); + bprintf(ctx.vcl->state, "%s", av[3] + 1); } static void __match_proto__(cli_func_t) -- 2.4.3
From 6dc5f523f0bca4bbcebe0158d055883fb9badfd0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Thu, 10 Dec 2015 11:44:05 +0100 Subject: [PATCH 4/9] Wrap VCL event calls in a dedicated function --- bin/varnishd/cache/cache_vcl.c | 45 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index d26c9af..9aa1259 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -428,6 +428,39 @@ vcl_find(const char *name) return (NULL); } +static int +vcl_event(VRT_CTX, enum vcl_event_e ev) +{ + int retval; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + AN(ctx->handling); + AN(ctx->vcl); + + retval = ctx->vcl->conf->event_vcl(ctx, ev); + + switch (ev) { + case VCL_EVENT_LOAD: + AN(ctx->msg); + /* fall through */ + case VCL_EVENT_WARM: + case VCL_EVENT_USE: + break; +#define EV(event, message) \ + case event: \ + if (retval) \ + WRONG("A VMOD failed the VCL " message); \ + break; + EV(VCL_EVENT_COLD, "cool down"); + EV(VCL_EVENT_DISCARD, "discard"); +#undef EV + default: + WRONG("Unexpected VCL event"); + } + + return (retval); +} + static void vcl_set_state(VRT_CTX, const char *state) { @@ -448,7 +481,7 @@ vcl_set_state(VRT_CTX, const char *state) vcl->temp = vcl->refcount ? vcl_temp_cooling : vcl_temp_cold; - AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD)); + (void)vcl_event(ctx, VCL_EVENT_COLD); vcl_BackendEvent(vcl, VCL_EVENT_COLD); } else if (vcl->busy) @@ -465,7 +498,7 @@ vcl_set_state(VRT_CTX, const char *state) /* The VCL must first reach a stable cold state */ else if (vcl->temp != vcl_temp_cooling) { vcl->temp = vcl_temp_warm; - (void)vcl->conf->event_vcl(ctx, VCL_EVENT_WARM); + (void)vcl_event(ctx, VCL_EVENT_WARM); vcl_BackendEvent(vcl, VCL_EVENT_WARM); } break; @@ -515,13 +548,13 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) VSB_clear(vsb); ctx.msg = vsb; - i = vcl->conf->event_vcl(&ctx, VCL_EVENT_LOAD); + i = vcl_event(&ctx, VCL_EVENT_LOAD); AZ(VSB_finish(vsb)); if (i) { VCLI_Out(cli, "VCL \"%s\" Failed initialization", name); if (VSB_len(vsb)) VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb)); - AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_DISCARD)); + (void)vcl_event(&ctx, VCL_EVENT_DISCARD); vcl_KillBackends(vcl); VCL_Close(&vcl); VSB_delete(vsb); @@ -563,7 +596,7 @@ VCL_Nuke(struct vcl *vcl) ctx.method = VCL_MET_FINI; ctx.handling = &hand; ctx.vcl = vcl; - AZ(vcl->conf->event_vcl(&ctx, VCL_EVENT_DISCARD)); + (void)vcl_event(&ctx, VCL_EVENT_DISCARD); vcl_KillBackends(vcl); free(vcl->loaded_name); VCL_Close(&vcl); @@ -688,7 +721,7 @@ ccf_config_use(struct cli *cli, const char * const *av, void *priv) AN(vsb); ctx.msg = vsb; ctx.vcl = vcl; - i = vcl->conf->event_vcl(&ctx, VCL_EVENT_USE); + i = vcl_event(&ctx, VCL_EVENT_USE); AZ(VSB_finish(vsb)); if (i) { VCLI_Out(cli, "VCL \"%s\" Failed to activate", av[2]); -- 2.4.3
From 331aef352579e170590ecf01e28c782db9ac4de3 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Fri, 4 Dec 2015 11:45:16 +0100 Subject: [PATCH 5/9] Allow VMODs to fail a warm-up It is also possible to convey a message to the CLI. In case of a failure the VCL is cooled down. VCL_Load may now fail with either CLIS_PARAM or CLIS_CANT. --- bin/varnishd/cache/cache_vcl.c | 83 ++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index 9aa1259..0e21b78 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -441,9 +441,9 @@ vcl_event(VRT_CTX, enum vcl_event_e ev) switch (ev) { case VCL_EVENT_LOAD: + case VCL_EVENT_WARM: AN(ctx->msg); /* fall through */ - case VCL_EVENT_WARM: case VCL_EVENT_USE: break; #define EV(event, message) \ @@ -461,14 +461,18 @@ vcl_event(VRT_CTX, enum vcl_event_e ev) return (retval); } -static void +static int vcl_set_state(VRT_CTX, const char *state) { struct vcl *vcl; + int i = 0; ASSERT_CLI(); CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); AN(ctx->handling); + AN(ctx->vcl); + AN(state); + assert(ctx->msg != NULL || *state == '0'); vcl = ctx->vcl; AN(vcl->temp); @@ -498,16 +502,37 @@ vcl_set_state(VRT_CTX, const char *state) /* The VCL must first reach a stable cold state */ else if (vcl->temp != vcl_temp_cooling) { vcl->temp = vcl_temp_warm; - (void)vcl_event(ctx, VCL_EVENT_WARM); - vcl_BackendEvent(vcl, VCL_EVENT_WARM); + i = vcl_event(ctx, VCL_EVENT_WARM); + if (i == 0) + vcl_BackendEvent(vcl, VCL_EVENT_WARM); + else + AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_COLD)); } break; default: WRONG("Wrong enum state"); } + return (i); } -static int +static void +vcl_unload(VRT_CTX, struct cli *cli, const char *name, const char *step) +{ + struct vcl *vcl = ctx->vcl; + + AZ(VSB_finish(ctx->msg)); + VCLI_SetResult(cli, CLIS_CANT); + VCLI_Out(cli, "VCL \"%s\" Failed %s", name, step); + if (VSB_len(ctx->msg)) + VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(ctx->msg)); + AZ(vcl->conf->event_vcl(ctx, VCL_EVENT_DISCARD)); + vcl_KillBackends(vcl); + VCL_Close(&vcl); + VSB_clear(ctx->msg); + VSB_delete(ctx->msg); +} + +static void VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) { struct vcl *vcl; @@ -520,8 +545,9 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) vcl = vcl_find(name); if (vcl != NULL) { + VCLI_SetResult(cli, CLIS_PARAM); VCLI_Out(cli, "Config '%s' already loaded", name); - return (1); + return; } vsb = VSB_new_auto(); @@ -530,9 +556,10 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) vcl = VCL_Open(fn, vsb); if (vcl == NULL) { AZ(VSB_finish(vsb)); + VCLI_SetResult(cli, CLIS_PARAM); VCLI_Out(cli, "%s", VSB_data(vsb)); VSB_delete(vsb); - return (1); + return; } vcl->loaded_name = strdup(name); @@ -549,19 +576,20 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) VSB_clear(vsb); ctx.msg = vsb; i = vcl_event(&ctx, VCL_EVENT_LOAD); - AZ(VSB_finish(vsb)); if (i) { - VCLI_Out(cli, "VCL \"%s\" Failed initialization", name); - if (VSB_len(vsb)) - VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb)); - (void)vcl_event(&ctx, VCL_EVENT_DISCARD); - vcl_KillBackends(vcl); - VCL_Close(&vcl); - VSB_delete(vsb); - return (1); + vcl_unload(&ctx, cli, name, "initialization"); + return; } + VSB_clear(vsb); + i = vcl_set_state(&ctx, state); + if (i) { + assert(*state == '1'); + vcl_unload(&ctx, cli, name, "warmup"); + return; + } + VSB_clear(vsb); + VSB_finish(vsb); VSB_delete(vsb); - vcl_set_state(&ctx, state); bprintf(vcl->state, "%s", state + 1); assert(hand == VCL_RET_OK); VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name); @@ -572,7 +600,6 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) Lck_Unlock(&vcl_mtx); VSC_C_main->n_vcl++; VSC_C_main->n_vcl_avail++; - return (0); } /*-------------------------------------------------------------------- @@ -620,7 +647,7 @@ VCL_Poll(void) INIT_OBJ(&ctx, VRT_CTX_MAGIC); ctx.vcl = vcl; ctx.handling = &hand; - vcl_set_state(&ctx, "0"); + (void)vcl_set_state(&ctx, "0"); } if (vcl->discard && vcl->temp == vcl_temp_cold) VCL_Nuke(vcl); @@ -656,8 +683,7 @@ ccf_config_load(struct cli *cli, const char * const *av, void *priv) AZ(priv); ASSERT_CLI(); - if (VCL_Load(cli, av[2], av[3], av[4])) - VCLI_SetResult(cli, CLIS_PARAM); + VCL_Load(cli, av[2], av[3], av[4]); } static void __match_proto__(cli_func_t) @@ -667,6 +693,7 @@ ccf_config_state(struct cli *cli, const char * const *av, void *priv) unsigned hand; INIT_OBJ(&ctx, VRT_CTX_MAGIC); + ctx.msg = VSB_new_auto(); ctx.handling = &hand; (void)cli; @@ -676,8 +703,18 @@ ccf_config_state(struct cli *cli, const char * const *av, void *priv) AN(av[3]); ctx.vcl = vcl_find(av[2]); AN(ctx.vcl); // MGT ensures this - vcl_set_state(&ctx, av[3]); - bprintf(ctx.vcl->state, "%s", av[3] + 1); + if (vcl_set_state(&ctx, av[3]) == 0) { + bprintf(ctx.vcl->state, "%s", av[3] + 1); + VSB_delete(ctx.msg); + return; + } + VSB_finish(ctx.msg); + VCLI_SetResult(cli, CLIS_CANT); + VCLI_Out(cli, "Failed <vcl.state %s %s>", ctx.vcl->loaded_name, + av[3] + 1); + if (VSB_len(ctx.msg)) + VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(ctx.msg)); + VSB_delete(ctx.msg); } static void __match_proto__(cli_func_t) -- 2.4.3
From a2db2134df3c6ae6dfbc8914d8848bae26217c1e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 11:18:33 +0100 Subject: [PATCH 6/9] Make VMODs actually fail warm-ups The implementation is similar to the load/discard dance when a load fails. New VGC functions are introduced iff the VCL has at least one VMOD handling events. The generated code looks like this: static unsigned vgc_inistep; static unsigned vgc_warmupstep; ... static int VGC_Load(VRT_CTX) { ... } static int VGC_Discard(VRT_CTX) { ... } static int VGC_Warmup(VRT_CTX, enum vcl_event_e ev) { vgc_warmupstep = 0; /* 4 */ if (Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev)) return (1); vgc_warmupstep = 4; return (0); } static int VGC_Use(VRT_CTX, enum vcl_event_e ev) { /* 4 */ if (Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev)) return (1); return (0); } static int VGC_Cooldown(VRT_CTX, enum vcl_event_e ev) { int retval = 0; /* 4 */ if (vgc_warmupstep >= 4 && Vmod_debug_Func._event(ctx, &vmod_priv_debug, ev) != 0) retval = 1; return (retval); } static int VGC_Event(VRT_CTX, enum vcl_event_e ev) { if (ev == VCL_EVENT_LOAD) return(VGC_Load(ctx)); if (ev == VCL_EVENT_WARM) return(VGC_Warmup(ctx, ev)); if (ev == VCL_EVENT_USE) return(VGC_Use(ctx, ev)); if (ev == VCL_EVENT_COLD) return(VGC_Cooldown(ctx, ev)); if (ev == VCL_EVENT_DISCARD) return(VGC_Discard(ctx)); return (1); } However, if there are no VMODs handling events, no new functions shall be generated, leading to code looking like this: static unsigned vgc_inistep; static unsigned vgc_warmupstep; ... static int VGC_Load(VRT_CTX) { ... } static int VGC_Discard(VRT_CTX) { ... } static int VGC_Event(VRT_CTX, enum vcl_event_e ev) { if (ev == VCL_EVENT_LOAD) return(VGC_Load(ctx)); if (ev == VCL_EVENT_DISCARD) return(VGC_Discard(ctx)); (void)vgc_warmupstep; return (0); } --- doc/sphinx/reference/vmod.rst | 6 +++ lib/libvcc/vcc_compile.c | 96 +++++++++++++++++++++++++++++++++++++------ lib/libvcc/vcc_vmod.c | 3 +- 3 files changed, 90 insertions(+), 15 deletions(-) diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst index 57fa09c..12a199d 100644 --- a/doc/sphinx/reference/vmod.rst +++ b/doc/sphinx/reference/vmod.rst @@ -383,6 +383,12 @@ first with a ``VCL_EVENT_WARM`` event. Unless a user decides that a given VCL should always be warm, an inactive VMOD will eventually become cold and should manage resources accordingly. +An event function must return zero upon success. It is therefore possible to +fail an initialization, the ``VCL_EVENT_LOAD`` or ``VCL_EVENT_WARM`` events. +Should such a failure happen, a ``VCL_EVENT_DISCARD`` or ``VCL_EVENT_COLD`` +event to the VMODs that succeeded. The VMOD that failed will not receive this +event, and therefore must not be left half-initialized should a failure occur. + If your VMOD is running an asynchronous background job you can hold a reference to the VCL to prevent it from going cold too soon and get the same guarantees as backends with ongoing requests for instance. For that, you must acquire the diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c index e79597f..2d08d00 100644 --- a/lib/libvcc/vcc_compile.c +++ b/lib/libvcc/vcc_compile.c @@ -325,20 +325,22 @@ EmitCoordinates(const struct vcc *tl, struct vsb *vsb) /*-------------------------------------------------------------------- * Init/Fini/Event * - * We call Fini-s in the opposite order of init-s. - * Other events are called in same order as init-s, no matter which - * event it might be. + * We call DISCARD and COLD events in the opposite order of LOAD and + * WARM. */ static void EmitInitFini(const struct vcc *tl) { struct inifin *p; + unsigned has_event = 0; - Fh(tl, 0, "\nstatic unsigned vgc_inistep;\n"); + Fh(tl, 0, "\n"); + Fh(tl, 0, "static unsigned vgc_inistep;\n"); + Fh(tl, 0, "static unsigned vgc_warmupstep;\n"); /* - * INIT + * LOAD */ Fc(tl, 0, "\nstatic int\nVGC_Load(VRT_CTX)\n{\n\n"); Fc(tl, 0, "\tvgc_inistep = 0;\n\n"); @@ -349,6 +351,10 @@ EmitInitFini(const struct vcc *tl) Fc(tl, 0, "\t/* %u */\n%s\n", p->n, VSB_data(p->ini)); Fc(tl, 0, "\tvgc_inistep = %u;\n\n", p->n); VSB_delete(p->ini); + + AZ(VSB_finish(p->event)); + if (VSB_len(p->event)) + has_event = 1; } Fc(tl, 0, "\t(void)VGC_function_vcl_init(ctx);\n"); @@ -356,7 +362,7 @@ EmitInitFini(const struct vcc *tl) Fc(tl, 0, "}\n"); /* - * FINI + * DISCARD */ Fc(tl, 0, "\nstatic int\nVGC_Discard(VRT_CTX)\n{\n\n"); @@ -375,6 +381,66 @@ EmitInitFini(const struct vcc *tl) Fc(tl, 0, "\treturn(0);\n"); Fc(tl, 0, "}\n"); + if (has_event) { + /* + * WARM + */ + Fc(tl, 0, "\nstatic int\n"); + Fc(tl, 0, "VGC_Warmup(VRT_CTX, enum vcl_event_e ev)\n{\n\n"); + + Fc(tl, 0, "\tvgc_warmupstep = 0;\n\n"); + VTAILQ_FOREACH(p, &tl->inifin, list) { + assert(p->n > 0); + if (VSB_len(p->event)) { + Fc(tl, 0, "\t/* %u */\n", p->n); + Fc(tl, 0, "\tif (%s)\n", VSB_data(p->event)); + Fc(tl, 0, "\t\treturn (1);\n"); + Fc(tl, 0, "\tvgc_warmupstep = %u;\n\n", p->n); + } + } + + Fc(tl, 0, "\treturn (0);\n"); + Fc(tl, 0, "}\n"); + + /* + * USE (deprecated) + */ + Fc(tl, 0, "\nstatic int\n"); + Fc(tl, 0, "VGC_Use(VRT_CTX, enum vcl_event_e ev)\n{\n\n"); + + VTAILQ_FOREACH(p, &tl->inifin, list) { + assert(p->n > 0); + if (VSB_len(p->event)) { + Fc(tl, 0, "\t/* %u */\n", p->n); + Fc(tl, 0, "\tif (%s)\n", VSB_data(p->event)); + Fc(tl, 0, "\t\treturn (1);\n\n"); + } + } + + Fc(tl, 0, "\treturn (0);\n"); + Fc(tl, 0, "}\n"); + + /* + * COLD + */ + Fc(tl, 0, "\nstatic int\n"); + Fc(tl, 0, "VGC_Cooldown(VRT_CTX, enum vcl_event_e ev)\n{\n"); + Fc(tl, 0, "\tint retval = 0;\n\n"); + + VTAILQ_FOREACH_REVERSE(p, &tl->inifin, inifinhead, list) { + if (VSB_len(p->event)) { + Fc(tl, 0, "\t/* %u */\n", p->n); + Fc(tl, 0, "\tif (vgc_warmupstep >= %u &&\n", p->n); + Fc(tl, 0, "\t %s != 0)\n", VSB_data(p->event)); + Fc(tl, 0, "\t\tretval = 1;\n\n"); + } + VSB_delete(p->event); + } + + Fc(tl, 0, "\treturn (retval);\n"); + Fc(tl, 0, "}\n"); + } + /* * EVENTS */ @@ -383,16 +449,20 @@ EmitInitFini(const struct vcc *tl) Fc(tl, 0, "{\n"); Fc(tl, 0, "\tif (ev == VCL_EVENT_LOAD)\n"); Fc(tl, 0, "\t\treturn(VGC_Load(ctx));\n"); + if (has_event) { + Fc(tl, 0, "\tif (ev == VCL_EVENT_WARM)\n"); + Fc(tl, 0, "\t\treturn(VGC_Warmup(ctx, ev));\n"); + Fc(tl, 0, "\tif (ev == VCL_EVENT_USE)\n"); + Fc(tl, 0, "\t\treturn(VGC_Use(ctx, ev));\n"); + Fc(tl, 0, "\tif (ev == VCL_EVENT_COLD)\n"); + Fc(tl, 0, "\t\treturn(VGC_Cooldown(ctx, ev));\n"); + } Fc(tl, 0, "\tif (ev == VCL_EVENT_DISCARD)\n"); Fc(tl, 0, "\t\treturn(VGC_Discard(ctx));\n"); Fc(tl, 0, "\n"); - VTAILQ_FOREACH(p, &tl->inifin, list) { - AZ(VSB_finish(p->event)); - if (VSB_len(p->event)) - Fc(tl, 0, "\t/* %u */\n%s\n", p->n, VSB_data(p->event)); - VSB_delete(p->event); - } - Fc(tl, 0, "\treturn (0);\n"); + if (!has_event) + Fc(tl, 0, "\t(void)vgc_warmupstep;\n"); + Fc(tl, 0, "\treturn (%d);\n", has_event ? 1 : 0); Fc(tl, 0, "}\n"); } diff --git a/lib/libvcc/vcc_vmod.c b/lib/libvcc/vcc_vmod.c index 60d2ed2..6cc2645 100644 --- a/lib/libvcc/vcc_vmod.c +++ b/lib/libvcc/vcc_vmod.c @@ -205,8 +205,7 @@ vcc_ParseImport(struct vcc *tl) VSB_printf(ifp->fin, "\t\t(void)%s(ctx, &vmod_priv_%.*s,\n" "\t\t VCL_EVENT_DISCARD);\n", p, PF(mod)); - VSB_printf(ifp->event, - "\t(void)%s(ctx, &vmod_priv_%.*s, ev);\n", + VSB_printf(ifp->event, "\t%s(ctx, &vmod_priv_%.*s, ev)", p, PF(mod)); } else { sym = VCC_AddSymbolStr(tl, p, SYM_FUNC); -- 2.4.3
From 3cd3bf43308e7393e72f003c405f1fac362413b4 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 12:07:27 +0100 Subject: [PATCH 7/9] Catch a vcl.state failure on the manager side Don't update the state of the VCL to warm if it failed, and don't start the child if the active VCL failed to warm up. --- bin/varnishd/mgt/mgt.h | 2 +- bin/varnishd/mgt/mgt_child.c | 3 ++- bin/varnishd/mgt/mgt_vcl.c | 49 ++++++++++++++++++++++++---------------- bin/varnishtest/tests/v00044.vtc | 8 +++++++ 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/bin/varnishd/mgt/mgt.h b/bin/varnishd/mgt/mgt.h index 5fb12d4..4756fad 100644 --- a/bin/varnishd/mgt/mgt.h +++ b/bin/varnishd/mgt/mgt.h @@ -167,7 +167,7 @@ void mgt_vcc_init(void); void mgt_vcl_init(void); void mgt_vcc_default(struct cli *, const char *b_arg, const char *vclsrc, int Cflag); -int mgt_push_vcls_and_start(unsigned *status, char **p); +int mgt_push_vcls_and_start(struct cli *, unsigned *status, char **p); int mgt_has_vcl(void); extern char *mgt_cc_cmd; extern const char *mgt_vcl_dir; diff --git a/bin/varnishd/mgt/mgt_child.c b/bin/varnishd/mgt/mgt_child.c index 8a26eb4..ae01a91 100644 --- a/bin/varnishd/mgt/mgt_child.c +++ b/bin/varnishd/mgt/mgt_child.c @@ -418,7 +418,8 @@ mgt_launch_child(struct cli *cli) mgt_cli_start_child(child_cli_in, child_cli_out); child_pid = pid; - if (mgt_push_vcls_and_start(&u, &p)) { + if (mgt_push_vcls_and_start(cli, &u, &p)) { + VCLI_SetResult(cli, u); MGT_complain(C_ERR, "Child (%jd) Pushing vcls failed:\n%s", (intmax_t)child_pid, p); free(p); diff --git a/bin/varnishd/mgt/mgt_vcl.c b/bin/varnishd/mgt/mgt_vcl.c index 583f478..609aba4 100644 --- a/bin/varnishd/mgt/mgt_vcl.c +++ b/bin/varnishd/mgt/mgt_vcl.c @@ -123,12 +123,13 @@ mgt_has_vcl(void) return (!VTAILQ_EMPTY(&vclhead)); } -static void -mgt_vcl_setstate(struct vclprog *vp, const char *vs) +static int +mgt_vcl_setstate(struct cli *cli, struct vclprog *vp, const char *vs) { unsigned status, warm; double now; char *p; + int i; if (vs == vcl_state_auto) { assert(vp != active_vcl); @@ -143,7 +144,7 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs) warm = vs == vcl_state_warm ? 1 : 0; if (vp->warm == warm) - return; + return (0); vp->warm = warm; @@ -151,15 +152,19 @@ mgt_vcl_setstate(struct vclprog *vp, const char *vs) vp->go_cold = 0; if (child_pid < 0) - return; + return (0); - /* - * We ignore the result here so we don't croak if the child did. - */ - (void)mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n", + i = mgt_cli_askchild(&status, &p, "vcl.state %s %d%s\n", vp->name, vp->warm, vp->state); + if (i) { + AN(cli); + AN(vp->warm); + VCLI_SetResult(cli, status); + VCLI_Out(cli, "%s", p); + } free(p); + return (i); } /*--------------------------------------------------------------------*/ @@ -232,12 +237,15 @@ mgt_vcc_default(struct cli *cli, const char *b_arg, const char *vclsrc, /*--------------------------------------------------------------------*/ int -mgt_push_vcls_and_start(unsigned *status, char **p) +mgt_push_vcls_and_start(struct cli *cli, unsigned *status, char **p) { struct vclprog *vp; AN(active_vcl); - mgt_vcl_setstate(active_vcl, vcl_state_warm); + + /* The VCL has not been loaded yet, it cannot fail */ + AZ(mgt_vcl_setstate(cli, active_vcl, vcl_state_warm)); + VTAILQ_FOREACH(vp, &vclhead, list) { if (mgt_cli_askchild(status, p, "vcl.load \"%s\" %s %d%s\n", vp->name, vp->fname, vp->warm, vp->state)) @@ -328,7 +336,7 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv) bprintf(vp->state, "%s", "auto"); if (vp != active_vcl) { vp->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp, vcl_state_auto); + (void)mgt_vcl_setstate(cli, vp, vcl_state_auto); } } else if (!strcmp(av[3], "cold")) { if (vp == active_vcl) { @@ -337,10 +345,10 @@ mcf_vcl_state(struct cli *cli, const char * const *av, void *priv) return; } bprintf(vp->state, "%s", "auto"); - mgt_vcl_setstate(vp, vcl_state_cold); + (void)mgt_vcl_setstate(cli, vp, vcl_state_cold); } else if (!strcmp(av[3], "warm")) { - bprintf(vp->state, "%s", av[3]); - mgt_vcl_setstate(vp, vcl_state_warm); + if (mgt_vcl_setstate(cli, vp, vcl_state_warm) == 0) + bprintf(vp->state, "%s", av[3]); } else { VCLI_Out(cli, "State must be one of auto, cold or warm."); VCLI_SetResult(cli, CLIS_PARAM); @@ -360,20 +368,21 @@ mcf_vcl_use(struct cli *cli, const char * const *av, void *priv) return; if (vp == active_vcl) return; - mgt_vcl_setstate(vp, vcl_state_warm); + if (mgt_vcl_setstate(cli, vp, vcl_state_warm)) + return; if (child_pid >= 0 && mgt_cli_askchild(&status, &p, "vcl.use %s\n", av[2])) { VCLI_SetResult(cli, status); VCLI_Out(cli, "%s", p); vp->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp, vcl_state_auto); + (void)mgt_vcl_setstate(cli, vp, vcl_state_auto); } else { VCLI_Out(cli, "VCL '%s' now active", av[2]); vp2 = active_vcl; active_vcl = vp; if (vp2 != NULL) { vp2->go_cold = VTIM_mono(); - mgt_vcl_setstate(vp2, vcl_state_auto); + (void)mgt_vcl_setstate(cli, vp2, vcl_state_auto); } } free(p); @@ -395,9 +404,9 @@ mcf_vcl_discard(struct cli *cli, const char * const *av, void *priv) VCLI_Out(cli, "Cannot discard active VCL program\n"); return; } - mgt_vcl_setstate(vp, vcl_state_cold); + (void)mgt_vcl_setstate(cli, vp, vcl_state_cold); if (child_pid >= 0) { - /* If this fails the child is crashing, figure that later */ + /* XXX If this fails the child is crashing, figure that later */ (void)mgt_cli_askchild(&status, &p, "vcl.discard %s\n", av[2]); free(p); } @@ -441,7 +450,7 @@ mgt_vcl_poker(const struct vev *e, int what) e_poker->timeout = mgt_param.vcl_cooldown * .45; VTAILQ_FOREACH(vp, &vclhead, list) { if (vp != active_vcl) - mgt_vcl_setstate(vp, vcl_state_auto); + (void)mgt_vcl_setstate(NULL, vp, vcl_state_auto); } return (0); } diff --git a/bin/varnishtest/tests/v00044.vtc b/bin/varnishtest/tests/v00044.vtc index aa79e6a..c056d6c 100644 --- a/bin/varnishtest/tests/v00044.vtc +++ b/bin/varnishtest/tests/v00044.vtc @@ -83,3 +83,11 @@ delay .4 varnish v1 -expect VBE.vcl1.default.happy >= 0 delay 4 varnish v1 -expect !VBE.vcl1.default.happy + +# A VMOD's warm-up can fail +varnish v1 -cliok "param.set max_esi_depth 42" +varnish v1 -clierr 300 "vcl.state vcl1 warm" + +# A warm-up failure can also fail a child start +varnish v1 -cliok stop +varnish v1 -clierr 300 start -- 2.4.3
From 87ba9d51cc37602c2bb8d41a23773792b9b9da89 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 15:02:13 +0100 Subject: [PATCH 8/9] Simplify `vcl.use` by making it failsafe By the time we decide to switch to a VCL, it must be warm and usable. The deprecated VCL_EVENT_USE should not get in the way. --- bin/varnishd/cache/cache_vcl.c | 26 ++++++-------------------- lib/libvcc/vcc_compile.c | 3 ++- lib/libvmod_debug/vmod_debug.c | 1 + 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index 0e21b78..cc40c1f 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -443,14 +443,13 @@ vcl_event(VRT_CTX, enum vcl_event_e ev) case VCL_EVENT_LOAD: case VCL_EVENT_WARM: AN(ctx->msg); - /* fall through */ - case VCL_EVENT_USE: break; #define EV(event, message) \ case event: \ if (retval) \ WRONG("A VMOD failed the VCL " message); \ break; + EV(VCL_EVENT_USE, "use"); EV(VCL_EVENT_COLD, "cool down"); EV(VCL_EVENT_DISCARD, "discard"); #undef EV @@ -744,33 +743,20 @@ ccf_config_use(struct cli *cli, const char * const *av, void *priv) struct vcl *vcl; struct vrt_ctx ctx; unsigned hand = 0; - struct vsb *vsb; - int i; ASSERT_CLI(); + AN(cli); AZ(priv); vcl = vcl_find(av[2]); AN(vcl); // MGT ensures this assert(vcl->temp == vcl_temp_warm); // MGT ensures this INIT_OBJ(&ctx, VRT_CTX_MAGIC); ctx.handling = &hand; - vsb = VSB_new_auto(); - AN(vsb); - ctx.msg = vsb; ctx.vcl = vcl; - i = vcl_event(&ctx, VCL_EVENT_USE); - AZ(VSB_finish(vsb)); - if (i) { - VCLI_Out(cli, "VCL \"%s\" Failed to activate", av[2]); - if (VSB_len(vsb) > 0) - VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(vsb)); - VCLI_SetResult(cli, CLIS_CANT); - } else { - Lck_Lock(&vcl_mtx); - vcl_active = vcl; - Lck_Unlock(&vcl_mtx); - } - VSB_delete(vsb); + (void)vcl_event(&ctx, VCL_EVENT_USE); + Lck_Lock(&vcl_mtx); + vcl_active = vcl; + Lck_Unlock(&vcl_mtx); } static void __match_proto__(cli_func_t) diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c index 2d08d00..0c80c3e 100644 --- a/lib/libvcc/vcc_compile.c +++ b/lib/libvcc/vcc_compile.c @@ -326,7 +326,8 @@ EmitCoordinates(const struct vcc *tl, struct vsb *vsb) * Init/Fini/Event * * We call DISCARD and COLD events in the opposite order of LOAD and - * WARM. + * WARM. The child will panic if a USE event fails, since a WARM event + * leads to a usable state. */ static void diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c index a9e2cde..9485268 100644 --- a/lib/libvmod_debug/vmod_debug.c +++ b/lib/libvmod_debug/vmod_debug.c @@ -278,6 +278,7 @@ event_warm(VRT_CTX) VSL(SLT_Debug, 0, "%s: VCL_EVENT_WARM", VCL_Name(ctx->vcl)); + AN(ctx->msg); if (cache_param->max_esi_depth == 42) { VSB_printf(ctx->msg, "max_esi_depth is not the answer."); return (-1); -- 2.4.3
From 98a8f7c1acc0c7f3de910d6589bf30a18fd0b56b Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune <[email protected]> Date: Mon, 7 Dec 2015 17:06:36 +0100 Subject: [PATCH 9/9] Replace the VCL refcount by a self-desribing list Instead of counting the references, the VCL keeps track of them with up to 31 characters in the description. It is the VMOD's responsibility to keep track of the opaque struct vclref * and provide a meaningful user- friendly description. --- bin/varnishd/cache/cache_vcl.c | 80 ++++++++++++++++++++++++++++++++-------- bin/varnishtest/tests/v00045.vtc | 12 +++++- doc/sphinx/reference/vmod.rst | 22 +++++++++++ include/vrt.h | 5 ++- lib/libvmod_debug/vmod_debug.c | 43 ++++++++++++++++----- 5 files changed, 134 insertions(+), 28 deletions(-) diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c index cc40c1f..ee9a69b 100644 --- a/bin/varnishd/cache/cache_vcl.c +++ b/bin/varnishd/cache/cache_vcl.c @@ -62,10 +62,18 @@ struct vcl { char state[8]; char *loaded_name; unsigned busy; - unsigned refcount; unsigned discard; const char *temp; VTAILQ_HEAD(,backend) backend_list; + VTAILQ_HEAD(,vclref) ref_list; +}; + +struct vclref { + unsigned magic; +#define VCLREF_MAGIC 0x47fb6848 + const struct vcl *vcl; + VTAILQ_ENTRY(vclref) list; + char desc[32]; }; /* @@ -252,7 +260,7 @@ vcl_KillBackends(struct vcl *vcl) CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC); AZ(vcl->busy); - AZ(vcl->refcount); + assert(VTAILQ_EMPTY(&vcl->ref_list)); while (1) { be = VTAILQ_FIRST(&vcl->backend_list); if (be == NULL) @@ -375,40 +383,59 @@ VRT_count(VRT_CTX, unsigned u) ctx->vcl->conf->ref[u].line, ctx->vcl->conf->ref[u].pos); } -void -VRT_ref_vcl(VRT_CTX) +struct vclref * +VRT_ref_vcl(VRT_CTX, const char *desc) { struct vcl *vcl; + struct vclref* ref; ASSERT_CLI(); CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + AN(desc); + AN(*desc); vcl = ctx->vcl; CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC); xxxassert(vcl->temp == vcl_temp_warm); + ALLOC_OBJ(ref, VCLREF_MAGIC); + AN(ref); + ref->vcl = vcl; + snprintf(ref->desc, sizeof ref->desc, "%s", desc); + Lck_Lock(&vcl_mtx); - vcl->refcount++; + VTAILQ_INSERT_TAIL(&vcl->ref_list, ref, list); Lck_Unlock(&vcl_mtx); + + return (ref); } void -VRT_rel_vcl(VRT_CTX) +VRT_rel_vcl(VRT_CTX, struct vclref **refp) { struct vcl *vcl; + struct vclref *ref; + + AN(refp); + ref = *refp; + *refp = NULL; CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + CHECK_OBJ_NOTNULL(ref, VCLREF_MAGIC); vcl = ctx->vcl; CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC); + assert(vcl == ref->vcl); assert(vcl->temp == vcl_temp_warm || vcl->temp == vcl_temp_busy || vcl->temp == vcl_temp_cooling); Lck_Lock(&vcl_mtx); - assert(vcl->refcount > 0); - vcl->refcount--; + assert(!VTAILQ_EMPTY(&vcl->ref_list)); + VTAILQ_REMOVE(&vcl->ref_list, ref, list); /* No garbage collection here, for the same reasons as in VCL_Rel. */ Lck_Unlock(&vcl_mtx); + + FREE_OBJ(ref); } /*--------------------------------------------------------------------*/ @@ -460,6 +487,23 @@ vcl_event(VRT_CTX, enum vcl_event_e ev) return (retval); } +static void +vcl_print_refs(VRT_CTX) +{ + struct vcl *vcl; + struct vclref *ref; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + CHECK_OBJ_NOTNULL(ctx->vcl, VCL_MAGIC); + AN(ctx->msg); + vcl = ctx->vcl; + VSB_printf(ctx->msg, "VCL %s is waiting for:", vcl->loaded_name); + Lck_Lock(&vcl_mtx); + VTAILQ_FOREACH(ref, &ctx->vcl->ref_list, list) + VSB_printf(ctx->msg, "\n\t- %s", ref->desc); + Lck_Unlock(&vcl_mtx); +} + static int vcl_set_state(VRT_CTX, const char *state) { @@ -482,16 +526,17 @@ vcl_set_state(VRT_CTX, const char *state) if (vcl->busy == 0 && (vcl->temp == vcl_temp_warm || vcl->temp == vcl_temp_busy)) { - vcl->temp = vcl->refcount ? vcl_temp_cooling : - vcl_temp_cold; + vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ? + vcl_temp_cold : vcl_temp_cooling; (void)vcl_event(ctx, VCL_EVENT_COLD); vcl_BackendEvent(vcl, VCL_EVENT_COLD); } else if (vcl->busy) vcl->temp = vcl_temp_busy; + else if (VTAILQ_EMPTY(&vcl->ref_list)) + vcl->temp = vcl_temp_cold; else - vcl->temp = vcl->refcount ? vcl_temp_cooling : - vcl_temp_cold; + vcl->temp = vcl_temp_cooling; break; case '1': assert(vcl->temp != vcl_temp_warm); @@ -499,7 +544,11 @@ vcl_set_state(VRT_CTX, const char *state) if (vcl->temp == vcl_temp_busy) vcl->temp = vcl_temp_warm; /* The VCL must first reach a stable cold state */ - else if (vcl->temp != vcl_temp_cooling) { + else if (vcl->temp == vcl_temp_cooling) { + vcl_print_refs(ctx); + i = -1; + } + else { vcl->temp = vcl_temp_warm; i = vcl_event(ctx, VCL_EVENT_WARM); if (i == 0) @@ -564,6 +613,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state) vcl->loaded_name = strdup(name); XXXAN(vcl->loaded_name); VTAILQ_INIT(&vcl->backend_list); + VTAILQ_INIT(&vcl->ref_list); vcl->temp = vcl_temp_init; @@ -617,7 +667,7 @@ VCL_Nuke(struct vcl *vcl) assert(vcl != vcl_active); assert(vcl->discard); AZ(vcl->busy); - AZ(vcl->refcount); + assert(VTAILQ_EMPTY(&vcl->ref_list)); VTAILQ_REMOVE(&vcl_head, vcl, list); ctx.method = VCL_MET_FINI; ctx.handling = &hand; @@ -733,7 +783,7 @@ ccf_config_discard(struct cli *cli, const char * const *av, void *priv) vcl->discard = 1; Lck_Unlock(&vcl_mtx); - if (vcl->busy == 0 && vcl->refcount == 0) + if (vcl->temp == vcl_temp_cold) VCL_Nuke(vcl); } diff --git a/bin/varnishtest/tests/v00045.vtc b/bin/varnishtest/tests/v00045.vtc index 1cc1259..936dec9 100644 --- a/bin/varnishtest/tests/v00045.vtc +++ b/bin/varnishtest/tests/v00045.vtc @@ -6,7 +6,7 @@ server s1 -start varnish v1 -vcl+backend { import ${vmod_debug}; sub vcl_init { - debug.vcl_release_delay(2s); + debug.vcl_release_delay(3s); } } -start @@ -21,9 +21,19 @@ shell { grep "auto/cooling.*vcl1" >/dev/null } +# It can't be warmed up yet +delay 1 +shell { + ${varnishadm} -n ${v1_name} vcl.state vcl1 warm 2>/dev/null | + grep "vmod-debug ref on vcl1" >/dev/null +} + # It will eventually cool down delay 2 shell { ${varnishadm} -n ${v1_name} vcl.list | grep "auto/cold.*vcl1" >/dev/null } + +# At this point it becomes possible to warm up again +varnish v1 -cliok "vcl.state vcl1 warm" diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst index 12a199d..47cb0ce 100644 --- a/doc/sphinx/reference/vmod.rst +++ b/doc/sphinx/reference/vmod.rst @@ -396,6 +396,28 @@ reference by calling ``VRT_ref_vcl`` when you receive a ``VCL_EVENT_WARM`` and later calling ``VRT_rel_vcl`` once the background job is over. Receiving a ``VCL_EVENT_COLD`` is your cue to terminate any background job bound to a VCL. +You can find an example of VCL references in vmod-debug:: + + priv_vcl->vclref = VRT_ref_vcl(ctx, "vmod-debug"); + ... + VRT_rel_vcl(&ctx, &priv_vcl->vclref); + +In this simplified version, you can see that you need at least a VCL-bound data +structure like a ``PRIV_VCL`` or a VMOD object to keep track of the reference +and later release it. You also have to provide a description, it will be printed +to the user if they try to warm up a cooling VCL:: + + $ varnishadm vcl.list + available auto/cooling 0 vcl1 + active auto/warm 0 vcl2 + + $ varnishadm vcl.state vcl1 warm + Command failed with error code 300 + Failed <vcl.state vcl1 auto> + Message: + VCL vcl1 is waiting for: + - vmod-debug + In the case where properly releasing resources may take some time, you can opt for an asynchronous worker, either by spawning a thread and tracking it, or by using Varnish's worker pools. diff --git a/include/vrt.h b/include/vrt.h index 96a3c64..4fe3067 100644 --- a/include/vrt.h +++ b/include/vrt.h @@ -298,8 +298,9 @@ struct vmod_priv { typedef int vmod_event_f(VRT_CTX, struct vmod_priv *, enum vcl_event_e); #endif -void VRT_ref_vcl(VRT_CTX); -void VRT_rel_vcl(VRT_CTX); +struct vclref; +struct vclref * VRT_ref_vcl(VRT_CTX, const char *); +void VRT_rel_vcl(VRT_CTX, struct vclref **); void VRT_priv_fini(const struct vmod_priv *p); struct vmod_priv *VRT_priv_task(VRT_CTX, void *vmod_id); diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c index 9485268..76287df 100644 --- a/lib/libvmod_debug/vmod_debug.c +++ b/lib/libvmod_debug/vmod_debug.c @@ -45,6 +45,8 @@ struct priv_vcl { #define PRIV_VCL_MAGIC 0x8E62FA9D char *foo; uintptr_t exp_cb; + struct vcl *vcl; + struct vclref *vclref; }; static VCL_DURATION vcl_release_delay = 0.0; @@ -248,6 +250,8 @@ priv_vcl_free(void *priv) EXP_Deregister_Callback(&priv_vcl->exp_cb); VSL(SLT_Debug, 0, "exp_cb: deregistered"); } + AZ(priv_vcl->vcl); + AZ(priv_vcl->vclref); FREE_OBJ(priv_vcl); AZ(priv_vcl); } @@ -273,8 +277,10 @@ event_load(VRT_CTX, struct vmod_priv *priv) } static int -event_warm(VRT_CTX) +event_warm(VRT_CTX, struct vmod_priv *priv) { + struct priv_vcl *priv_vcl; + char buf[32]; VSL(SLT_Debug, 0, "%s: VCL_EVENT_WARM", VCL_Name(ctx->vcl)); @@ -284,7 +290,13 @@ event_warm(VRT_CTX) return (-1); } - VRT_ref_vcl(ctx); + CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC); + AZ(priv_vcl->vcl); + AZ(priv_vcl->vclref); + + snprintf(buf, sizeof buf, "vmod-debug ref on %s", VCL_Name(ctx->vcl)); + priv_vcl->vcl = ctx->vcl; + priv_vcl->vclref = VRT_ref_vcl(ctx, buf); return (0); } @@ -292,29 +304,40 @@ static void* cooldown_thread(void *priv) { struct vrt_ctx ctx; + struct priv_vcl *priv_vcl; + + CAST_OBJ_NOTNULL(priv_vcl, priv, PRIV_VCL_MAGIC); + AN(priv_vcl->vcl); + AN(priv_vcl->vclref); - AN(priv); INIT_OBJ(&ctx, VRT_CTX_MAGIC); - ctx.vcl = (struct vcl*)priv; + ctx.vcl = priv_vcl->vcl; VTIM_sleep(vcl_release_delay); - VRT_rel_vcl(&ctx); + VRT_rel_vcl(&ctx, &priv_vcl->vclref); + priv_vcl->vcl = NULL; return (NULL); } static int -event_cold(VRT_CTX) +event_cold(VRT_CTX, struct vmod_priv *priv) { pthread_t thread; + struct priv_vcl *priv_vcl; + + CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC); + AN(priv_vcl->vcl); + AN(priv_vcl->vclref); VSL(SLT_Debug, 0, "%s: VCL_EVENT_COLD", VCL_Name(ctx->vcl)); if (vcl_release_delay == 0.0) { - VRT_rel_vcl(ctx); + VRT_rel_vcl(ctx, &priv_vcl->vclref); + priv_vcl->vcl = NULL; return (0); } - AZ(pthread_create(&thread, NULL, cooldown_thread, ctx->vcl)); + AZ(pthread_create(&thread, NULL, cooldown_thread, priv_vcl)); AZ(pthread_detach(thread)); return (0); } @@ -325,8 +348,8 @@ event_function(VRT_CTX, struct vmod_priv *priv, enum vcl_event_e e) switch (e) { case VCL_EVENT_LOAD: return event_load(ctx, priv); - case VCL_EVENT_COLD: return event_cold(ctx); - case VCL_EVENT_WARM: return event_warm(ctx); + case VCL_EVENT_WARM: return event_warm(ctx, priv); + case VCL_EVENT_COLD: return event_cold(ctx, priv); default: return (0); } } -- 2.4.3
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
