[master] 3cfd35136 Fix ctx->msg for CLI COLD events and simplify VRT_CTX management
Nils Goroll
nils.goroll at uplex.de
Mon Mar 2 15:22:06 UTC 2020
commit 3cfd35136d3e80a81507e3d5cc11193c8ebbf343
Author: Nils Goroll <nils.goroll at uplex.de>
Date: Thu Feb 13 21:12:10 2020 +0100
Fix ctx->msg for CLI COLD events and simplify VRT_CTX management
This commit adds the final bit to fix #2902: As discussed in the
ticket, there should be no VRT_CTX msg vsb present for COLD events,
yet existing code did provide it in some cases.
This commit message comments on changes in cache_vcl.c in order of
appearance.
Existing code kept the CLI VRT_CTX around for longer than it strictly
needed to just because access to the error message in ctx->msg was
still required.
In order to be able to simplify management of the CLI VRT_CTX, we
change VCL_Rel_CliCtx() to return the ctx->msg IFF it was requested by
a true argument to VCL_Get_CliCtx()
Regarding #2902, the main issue with existing code was that the
decision on whether or not to request a ctx->msg was separated from
where we could actually make that decision: It is (only) in
vcl_send_event() where we know which event is going to be issued,
which, in turn, determines if we are going to need a ctx->msg.
Thus, we move the VRT_CTX management for all of the CLI operations
into one place in vcl_send_event() and change the infrastructure
around it to handle the vcl and the error message vsb instead of a
VRT_CTX.
This allows us to (finally) ensure that VCL_EVENT_COLD does never have
a ctx->msg error vsb, asserted by AZ(havemsg) in the switch/case
statement in vcl_send_event().
In vcl_send_event(), we also assert that if a vcl subroutine was
called in the case of a LOAD or DISCARD event, only vcl_init {} for a
LOAD event may return anything but OK.
vcl_set_state() contains the two use cases of vcl_send_event(): When
we know that an event may not fail, we also assert that there is no
msg vsb returned, and if it could, we keep it, as it is still required
by its callers.
vcl_cancel_load() shows how the new infrastructure simplifies the
code: Rather than having to re-setup the VRT_CTX for the DISCARD
event, we simply output the error and have vcl_send_event() set up the
proper handling for that event.
Basically the same pattern repeats in vcl_load(), VCL_Poll() and
vcl_cli_state(): In these functions, we only look after error handling
and output.
Fixes #2902
diff --git a/bin/varnishd/cache/cache_director.c b/bin/varnishd/cache/cache_director.c
index b0adeb172..fe5b132b9 100644
--- a/bin/varnishd/cache/cache_director.c
+++ b/bin/varnishd/cache/cache_director.c
@@ -344,7 +344,7 @@ do_list(struct cli *cli, struct director *d, void *priv)
d->vdir->methods->list(ctx, d, la->vsb, la->p, 0);
VSB_printf(la->vsb, "\n");
- VCL_Rel_CliCtx(&ctx);
+ AZ(VCL_Rel_CliCtx(&ctx));
AZ(ctx);
return (0);
@@ -389,7 +389,7 @@ do_list_json(struct cli *cli, struct director *d, void *priv)
VSB_indent(cli->sb, -2);
VCLI_Out(cli, "}");
- VCL_Rel_CliCtx(&ctx);
+ AZ(VCL_Rel_CliCtx(&ctx));
AZ(ctx);
return (0);
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index e7fc7515f..6e98f425d 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -430,7 +430,7 @@ const char *VCL_Method_Name(unsigned);
void VCL_Bo2Ctx(struct vrt_ctx *, struct busyobj *);
void VCL_Req2Ctx(struct vrt_ctx *, struct req *);
struct vrt_ctx *VCL_Get_CliCtx(int);
-void VCL_Rel_CliCtx(struct vrt_ctx **);
+struct vsb *VCL_Rel_CliCtx(struct vrt_ctx **);
#define VCL_MET_MAC(l,u,t,b) \
void VCL_##l##_method(struct vcl *, struct worker *, struct req *, \
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index dba873d0b..fe8758a6b 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -140,62 +140,91 @@ VCL_Get_CliCtx(int msg)
return (&ctx_cli);
}
-void
+/*
+ * releases CLI ctx
+ *
+ * returns finished error msg vsb if VCL_Get_CliCtx(1) was called
+ *
+ * caller needs to VSB_destroy a non-NULL return value
+ *
+ */
+struct vsb *
VCL_Rel_CliCtx(struct vrt_ctx **ctx)
{
+ struct vsb *r = NULL;
ASSERT_CLI();
assert(*ctx == &ctx_cli);
AN((*ctx)->handling);
- if (ctx_cli.msg)
- VSB_destroy(&ctx_cli.msg);
+ if (ctx_cli.msg) {
+ TAKE_OBJ_NOTNULL(r, &ctx_cli.msg, VSB_MAGIC);
+ AZ(VSB_finish(r));
+ }
if (ctx_cli.vsl)
VSL_Flush(ctx_cli.vsl, 0);
WS_Assert(ctx_cli.ws);
WS_Reset(&ws_cli, ws_snapshot_cli);
INIT_OBJ(*ctx, VRT_CTX_MAGIC);
*ctx = NULL;
+
+ return (r);
}
/*--------------------------------------------------------------------*/
static int
-vcl_send_event(struct vrt_ctx *ctx, enum vcl_event_e ev)
+vcl_send_event(struct vcl *vcl, enum vcl_event_e ev, struct vsb **msg)
{
- int r;
+ int r, havemsg = 0;
+ unsigned method = 0;
+ struct vrt_ctx *ctx;
ASSERT_CLI();
- CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->vcl, VCL_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->vcl->conf, VCL_CONF_MAGIC);
- AZ(ctx->method);
+ CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+ CHECK_OBJ_NOTNULL(vcl->conf, VCL_CONF_MAGIC);
+ AN(msg);
+ AZ(*msg);
+
switch (ev) {
case VCL_EVENT_LOAD:
- ctx->method = VCL_MET_INIT;
+ method = VCL_MET_INIT;
/* FALLTHROUGH */
case VCL_EVENT_WARM:
- AN(ctx->msg);
+ havemsg = 1;
break;
case VCL_EVENT_DISCARD:
- ctx->method = VCL_MET_FINI;
+ method = VCL_MET_FINI;
/* FALLTHROUGH */
case VCL_EVENT_COLD:
- // XXX AZ(ctx->msg);
+ AZ(havemsg);
break;
default:
WRONG("vcl_event");
}
+ ctx = VCL_Get_CliCtx(havemsg);
+
AN(ctx->handling);
- *ctx->handling = 0;
+ AZ(*ctx->handling);
AN(ctx->ws);
+ ctx->vcl = vcl;
+ ctx->syntax = ctx->vcl->conf->syntax;
+ ctx->method = method;
+
VCL_TaskEnter(cli_task_privs);
r = ctx->vcl->conf->event_vcl(ctx, ev);
VCL_TaskLeave(cli_task_privs);
- ctx->method = 0;
+ /* if the warm event did not get to vcl_init, vcl_fini
+ * won't be run, so handling may be zero */
+ if (method && *ctx->handling && *ctx->handling != VCL_RET_OK) {
+ assert(ev == VCL_EVENT_LOAD);
+ r = 1;
+ }
+
+ *msg = VCL_Rel_CliCtx(&ctx);
if (r && (ev == VCL_EVENT_COLD || ev == VCL_EVENT_DISCARD))
WRONG("A VMOD cannot fail COLD or DISCARD events");
@@ -499,39 +528,37 @@ VCL_TestLoad(const char *fn)
/*--------------------------------------------------------------------*/
-static void
-vcl_print_refs(VRT_CTX)
+static struct vsb *
+vcl_print_refs(const struct vcl *vcl)
{
- struct vcl *vcl;
+ struct vsb *msg;
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);
+ CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+ msg = VSB_new_auto();
+
+ VSB_printf(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);
+ VTAILQ_FOREACH(ref, &vcl->ref_list, list)
+ VSB_printf(msg, "\n\t- %s", ref->desc);
Lck_Unlock(&vcl_mtx);
+ AZ(VSB_finish(msg));
+ return (msg);
}
static int
-vcl_set_state(struct vrt_ctx *ctx, const char *state)
+vcl_set_state(struct vcl *vcl, const char *state, struct vsb **msg)
{
- struct vcl *vcl;
+ struct vsb *nomsg = NULL;
int i = 0;
ASSERT_CLI();
- CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->vcl, VCL_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->vcl->conf, VCL_CONF_MAGIC);
- AN(ctx->handling);
- AN(ctx->vcl);
+
+ CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
AN(state);
- assert(ctx->msg != NULL || *state == '0');
+ AN(msg);
+ AZ(*msg);
- vcl = ctx->vcl;
AN(vcl->temp);
switch (state[0]) {
@@ -541,7 +568,8 @@ vcl_set_state(struct vrt_ctx *ctx, const char *state)
if (vcl->busy == 0 && vcl->temp->is_warm) {
vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ?
VCL_TEMP_COLD : VCL_TEMP_COOLING;
- AZ(vcl_send_event(ctx, VCL_EVENT_COLD));
+ AZ(vcl_send_event(vcl, VCL_EVENT_COLD, msg));
+ AZ(*msg);
vcl_BackendEvent(vcl, VCL_EVENT_COLD);
}
else if (vcl->busy)
@@ -559,17 +587,18 @@ vcl_set_state(struct vrt_ctx *ctx, const char *state)
vcl->temp = VCL_TEMP_WARM;
/* The VCL must first reach a stable cold state */
else if (vcl->temp == VCL_TEMP_COOLING) {
- vcl_print_refs(ctx);
+ *msg = vcl_print_refs(vcl);
i = -1;
}
else {
vcl->temp = VCL_TEMP_WARM;
- i = vcl_send_event(ctx, VCL_EVENT_WARM);
+ i = vcl_send_event(vcl, VCL_EVENT_WARM, msg);
if (i == 0) {
vcl_BackendEvent(vcl, VCL_EVENT_WARM);
break;
}
- AZ(vcl_send_event(ctx, VCL_EVENT_COLD));
+ AZ(vcl_send_event(vcl, VCL_EVENT_COLD, &nomsg));
+ AZ(nomsg);
vcl->temp = VCL_TEMP_COLD;
}
break;
@@ -583,36 +612,33 @@ vcl_set_state(struct vrt_ctx *ctx, const char *state)
}
static void
-vcl_cancel_load(struct vrt_ctx *ctx, struct cli *cli,
+vcl_cancel_load(struct vcl *vcl, struct cli *cli, struct vsb *msg,
const char *name, const char *step)
{
- struct vcl *vcl = ctx->vcl;
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
CHECK_OBJ_NOTNULL(vcl->conf, VCL_CONF_MAGIC);
- 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));
- VCL_Rel_CliCtx(&ctx);
- ctx = VCL_Get_CliCtx(0);
- ctx->vcl = vcl;
- ctx->syntax = ctx->vcl->conf->syntax;
- AZ(vcl_send_event(ctx, VCL_EVENT_DISCARD));
+ if (VSB_len(msg))
+ VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(msg));
+ VSB_destroy(&msg);
+
+ AZ(vcl_send_event(vcl, VCL_EVENT_DISCARD, &msg));
+ AZ(msg);
+
vcl_KillBackends(vcl);
free(vcl->loaded_name);
VCL_Close(&vcl);
}
static void
-vcl_load(struct cli *cli, struct vrt_ctx *ctx,
+vcl_load(struct cli *cli,
const char *name, const char *fn, const char *state)
{
struct vcl *vcl;
struct vsb *msg;
- int i;
ASSERT_CLI();
@@ -621,10 +647,10 @@ vcl_load(struct cli *cli, struct vrt_ctx *ctx,
msg = VSB_new_auto();
vcl = VCL_Open(fn, msg);
- AZ(VSB_finish(ctx->msg));
+ AZ(VSB_finish(msg));
if (vcl == NULL) {
VCLI_SetResult(cli, CLIS_PARAM);
- VCLI_Out(cli, "%s", VSB_data(ctx->msg));
+ VCLI_Out(cli, "%s", VSB_data(msg));
VSB_destroy(&msg);
return;
}
@@ -640,23 +666,20 @@ vcl_load(struct cli *cli, struct vrt_ctx *ctx,
vcl->temp = VCL_TEMP_INIT;
- ctx->vcl = vcl;
- ctx->syntax = ctx->vcl->conf->syntax;
-
- VSB_clear(ctx->msg);
- i = vcl_send_event(ctx, VCL_EVENT_LOAD);
- if (i || *ctx->handling != VCL_RET_OK) {
- vcl_cancel_load(ctx, cli, name, "initialization");
+ if (vcl_send_event(vcl, VCL_EVENT_LOAD, &msg)) {
+ vcl_cancel_load(vcl, cli, msg, name, "initialization");
return;
}
- assert(*ctx->handling == VCL_RET_OK);
- VSB_clear(ctx->msg);
- i = vcl_set_state(ctx, state);
- if (i) {
+ VSB_destroy(&msg);
+
+ if (vcl_set_state(vcl, state, &msg)) {
assert(*state == '1');
- vcl_cancel_load(ctx, cli, name, "warmup");
+ vcl_cancel_load(vcl, cli, msg, name, "warmup");
return;
}
+ if (msg)
+ VSB_destroy(&msg);
+
VCLI_Out(cli, "Loaded \"%s\" as \"%s\"", fn , name);
VTAILQ_INSERT_TAIL(&vcl_head, vcl, list);
Lck_Lock(&vcl_mtx);
@@ -672,36 +695,27 @@ vcl_load(struct cli *cli, struct vrt_ctx *ctx,
void
VCL_Poll(void)
{
- struct vrt_ctx *ctx;
+ struct vsb *nomsg = NULL;
struct vcl *vcl, *vcl2;
ASSERT_CLI();
VTAILQ_FOREACH_SAFE(vcl, &vcl_head, list, vcl2) {
if (vcl->temp == VCL_TEMP_BUSY ||
- vcl->temp == VCL_TEMP_COOLING) {
- // XXX #2902 : cold event to have msg ?
- // move ctx into vcl_set_state() ?
- ctx = VCL_Get_CliCtx(1);
- ctx->vcl = vcl;
- ctx->syntax = ctx->vcl->conf->syntax;
- (void)vcl_set_state(ctx, "0");
- VCL_Rel_CliCtx(&ctx);
- }
+ vcl->temp == VCL_TEMP_COOLING)
+ AZ(vcl_set_state(vcl, "0", &nomsg));
+ AZ(nomsg);
if (vcl->discard && vcl->temp == VCL_TEMP_COLD) {
AZ(vcl->busy);
assert(vcl != vcl_active);
assert(VTAILQ_EMPTY(&vcl->ref_list));
VTAILQ_REMOVE(&vcl_head, vcl, list);
- ctx = VCL_Get_CliCtx(0);
- ctx->vcl = vcl;
- ctx->syntax = ctx->vcl->conf->syntax;
- AZ(vcl_send_event(ctx, VCL_EVENT_DISCARD));
+ AZ(vcl_send_event(vcl, VCL_EVENT_DISCARD, &nomsg));
+ AZ(nomsg);
vcl_KillBackends(vcl);
free(vcl->loaded_name);
VCL_Close(&vcl);
VSC_C_main->n_vcl--;
VSC_C_main->n_vcl_discard--;
- VCL_Rel_CliCtx(&ctx);
}
}
}
@@ -794,37 +808,38 @@ vcl_cli_list_json(struct cli *cli, const char * const *av, void *priv)
static void v_matchproto_(cli_func_t)
vcl_cli_load(struct cli *cli, const char * const *av, void *priv)
{
- struct vrt_ctx *ctx;
AZ(priv);
ASSERT_CLI();
- ctx = VCL_Get_CliCtx(1);
- vcl_load(cli, ctx, av[2], av[3], av[4]);
- VCL_Rel_CliCtx(&ctx);
+ // XXX move back code from vcl_load?
+ vcl_load(cli, av[2], av[3], av[4]);
}
static void v_matchproto_(cli_func_t)
vcl_cli_state(struct cli *cli, const char * const *av, void *priv)
{
- struct vrt_ctx *ctx;
+ struct vcl *vcl;
+ struct vsb *msg = NULL;
AZ(priv);
ASSERT_CLI();
AN(av[2]);
AN(av[3]);
- ctx = VCL_Get_CliCtx(1);
- ctx->vcl = vcl_find(av[2]);
- AN(ctx->vcl);
- ctx->syntax = ctx->vcl->conf->syntax;
- if (vcl_set_state(ctx, av[3])) {
- AZ(VSB_finish(ctx->msg));
+
+ vcl = vcl_find(av[2]);
+ AN(vcl);
+
+ if (vcl_set_state(vcl, av[3], &msg)) {
+ CHECK_OBJ_NOTNULL(msg, VSB_MAGIC);
+
VCLI_SetResult(cli, CLIS_CANT);
- VCLI_Out(cli, "Failed <vcl.state %s %s>", ctx->vcl->loaded_name,
+ VCLI_Out(cli, "Failed <vcl.state %s %s>", vcl->loaded_name,
av[3] + 1);
- if (VSB_len(ctx->msg))
- VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(ctx->msg));
+ if (VSB_len(msg))
+ VCLI_Out(cli, "\nMessage:\n\t%s", VSB_data(msg));
}
- VCL_Rel_CliCtx(&ctx);
+ if (msg)
+ VSB_destroy(&msg);
}
static void v_matchproto_(cli_func_t)
diff --git a/include/vrt.h b/include/vrt.h
index 8d06bb7b5..0b54d74e5 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -292,7 +292,6 @@ struct vrt_ctx {
* msg is for error messages and exists only for
* VCL_EVENT_LOAD
* VCL_EVENT_WARM
- * VCL_EVENT_COLD
*/
struct vsb *msg;
struct vsl_log *vsl;
diff --git a/lib/libvmod_debug/vmod_debug.c b/lib/libvmod_debug/vmod_debug.c
index 45234400c..11fddb90a 100644
--- a/lib/libvmod_debug/vmod_debug.c
+++ b/lib/libvmod_debug/vmod_debug.c
@@ -499,7 +499,7 @@ event_cold(VRT_CTX, const struct vmod_priv *priv)
pthread_t thread;
struct priv_vcl *priv_vcl;
- AN(ctx->msg);
+ AZ(ctx->msg);
CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
More information about the varnish-commit
mailing list