Mailing List Archive

commit a8483cf5c864a87f59b2cc26315f37e9edda2d97
Author: Nils Goroll <nils.goroll@uplex.de>
Date: Mon Feb 1 17:27:12 2021 +0100

Give VPI its private struct in the worker

(struct vrt_ctx).handling is a special animal: As seen by VGC, struct
vrt_ctx is const (and rightly so), yet VCL needs to have a way to
communicate up to core code the return value from subs.

So, handling was a (non-const) pointer into struct worker. This is all
good and well, with the minor drawback that VMODs would get to see it,
while we should make it very clear that they have to go through
VRT_handling() / VRT_handled() and know the rules (no reset of
handling).

As we now want to move also the vpi count into "the ctx", before adding
another unsigned pointer, we introduce struct wrk_vpi, provided by the
worker, but owned by vpi.

With this change, we thus formalize the vpi count requirement and also
remove the handling variable from vmods' direct line of sight.

This commit comprises the manual changes which either could not be or
were not worth being automated.

The next two commits contain a coccinelle script to do the rest of the
work, and the reult of it.

diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index e74104388..af090b6fe 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -244,7 +244,8 @@ struct worker {

unsigned cur_method;
unsigned seen_methods;
- unsigned handling;
+
+ struct wrk_vpi *vpi;
};

/* Stored object -----------------------------------------------------
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 4dcf0d8fe..11b7e5044 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -38,6 +38,7 @@
#include "storage/storage.h"
#include "vcl.h"
#include "vtim.h"
+#include "vcc_interface.h"

#define FETCH_STEPS \
FETCH_STEP(mkbereq, MKBEREQ) \
diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c
index 658f927b0..dd288c1f6 100644
--- a/bin/varnishd/cache/cache_panic.c
+++ b/bin/varnishd/cache/cache_panic.c
@@ -43,6 +43,7 @@

#include "cache_varnishd.h"
#include "cache_transport.h"
+#include "vcc_interface.h"

#include "cache_filter.h"
#include "common/heritage.h"
diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index 56668026d..48f5d5c7b 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -44,6 +44,7 @@
#include "cache_filter.h"
#include "cache_objhead.h"
#include "cache_transport.h"
+#include "vcc_interface.h"

#include "hash/hash_slinger.h"
#include "http1/cache_http1.h"
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index b9d60fc65..eb3981da0 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -524,6 +524,10 @@ void WRK_Init(void);
void WRK_AddStat(const struct worker *);
void WRK_Log(enum VSL_tag_e, const char *, ...);

+/* cache_vpi.c */
+extern const size_t vpi_wrk_len;
+void VPI_wrk_init(struct worker *, void *, size_t);
+
/* cache_ws.c */
void WS_Panic(struct vsb *, const struct ws *);
static inline int
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 293c199ba..97490920d 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -68,7 +68,7 @@ struct lock vcl_mtx;
struct vcl *vcl_active; /* protected by vcl_mtx */

static struct vrt_ctx ctx_cli;
-static unsigned handling_cli;
+static struct wrk_vpi wrk_vpi_cli;
static struct ws ws_cli;
static uintptr_t ws_snapshot_cli;
static struct vsl_log vsl_cli;
@@ -125,10 +125,9 @@ VCL_Get_CliCtx(int msg)
{

ASSERT_CLI();
- AZ(ctx_cli.handling);
INIT_OBJ(&ctx_cli, VRT_CTX_MAGIC);
- handling_cli = 0;
- ctx_cli.handling = &handling_cli;
+ INIT_OBJ(&wrk_vpi_cli, WRK_VPI_MAGIC);
+ ctx_cli.vpi = &wrk_vpi_cli;
ctx_cli.now = VTIM_real();
if (msg) {
ctx_cli.msg = VSB_new_auto();
@@ -156,7 +155,7 @@ VCL_Rel_CliCtx(struct vrt_ctx **ctx)

ASSERT_CLI();
assert(*ctx == &ctx_cli);
- AN((*ctx)->handling);
+ AN((*ctx)->vpi);
if (ctx_cli.msg) {
TAKE_OBJ_NOTNULL(r, &ctx_cli.msg, VSB_MAGIC);
AZ(VSB_finish(r));
diff --git a/bin/varnishd/cache/cache_vpi.c b/bin/varnishd/cache/cache_vpi.c
index f6e09dd86..d2b149701 100644
--- a/bin/varnishd/cache/cache_vpi.c
+++ b/bin/varnishd/cache/cache_vpi.c
@@ -45,6 +45,20 @@
* Private & exclusive interfaces between VCC and varnishd
*/

+const size_t vpi_wrk_len = sizeof(struct wrk_vpi);
+
+void
+VPI_wrk_init(struct worker *wrk, void *p, size_t spc)
+{
+ struct wrk_vpi *vpi = p;
+
+ CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
+ AN(vpi);
+ assert(spc >= sizeof *vpi);
+ INIT_OBJ(vpi, WRK_VPI_MAGIC);
+ wrk->vpi = vpi;
+}
+
void
VPI_count(VRT_CTX, unsigned u)
{
diff --git a/bin/varnishd/cache/cache_vrt_priv.c b/bin/varnishd/cache/cache_vrt_priv.c
index 03e684834..685bb451c 100644
--- a/bin/varnishd/cache/cache_vrt_priv.c
+++ b/bin/varnishd/cache/cache_vrt_priv.c
@@ -38,6 +38,7 @@

#include "cache_varnishd.h"
#include "vcl.h"
+#include "vcc_interface.h"

struct vrt_priv {
unsigned magic;
diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c
index a71edc9a3..976930e4e 100644
--- a/bin/varnishd/cache/cache_vrt_vcl.c
+++ b/bin/varnishd/cache/cache_vrt_vcl.c
@@ -583,7 +583,7 @@ VCL_##func##_method(struct vcl *vcl, struct worker *wrk, \
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); \
vcl_call_method(wrk, req, bo, specific, \
VCL_MET_ ## upper, vcl->conf->func##_func, vcl->conf->nsub);\
- AN((1U << wrk->handling) & bitmap); \
+ AN((1U << wrk->vpi->handling) & bitmap); \
}

#include "tbl/vcl_returns.h"
diff --git a/bin/varnishd/cache/cache_wrk.c b/bin/varnishd/cache/cache_wrk.c
index 9671413b6..3a5c4bddd 100644
--- a/bin/varnishd/cache/cache_wrk.c
+++ b/bin/varnishd/cache/cache_wrk.c
@@ -96,6 +96,7 @@ wrk_bgthread(void *arg)
INIT_OBJ(&wrk, WORKER_MAGIC);
INIT_OBJ(wpriv, WORKER_PRIV_MAGIC);
wrk.wpriv = wpriv;
+ // bgthreads do not have a vpi member
memset(&ds, 0, sizeof ds);
wrk.stats = &ds;

@@ -126,6 +127,7 @@ WRK_Thread(struct pool *qp, size_t stacksize, unsigned thread_workspace)
struct VSC_main_wrk ds;
unsigned char ws[thread_workspace];
struct worker_priv wpriv[1];
+ unsigned char vpi[vpi_wrk_len];

AN(qp);
AN(stacksize);
@@ -143,6 +145,8 @@ WRK_Thread(struct pool *qp, size_t stacksize, unsigned thread_workspace)
AZ(pthread_cond_init(&w->cond, NULL));

WS_Init(w->aws, "wrk", ws, thread_workspace);
+ VPI_wrk_init(w, vpi, sizeof vpi);
+ AN(w->vpi);

VSL(SLT_WorkThread, 0, "%p start", w);

diff --git a/include/vcc_interface.h b/include/vcc_interface.h
index c1a74fc37..4a51868cd 100644
--- a/include/vcc_interface.h
+++ b/include/vcc_interface.h
@@ -53,6 +53,14 @@ struct vpi_ref {
const char *token;
};

+/* VPI's private part of the worker */
+struct wrk_vpi {
+ unsigned magic;
+#define WRK_VPI_MAGIC 0xaa3d3df3
+ unsigned handling;
+};
+
+
void VPI_count(VRT_CTX, unsigned);
void VPI_vcl_fini(VRT_CTX);

diff --git a/include/vrt.h b/include/vrt.h
index c4afdc6b6..e5e5647bf 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -279,6 +279,7 @@ struct VSC_main;
struct vsc_seg;
struct vsl_log;
struct vsmw_cluster;
+struct wrk_vpi;
struct ws;

typedef const struct stream_close *stream_close_t;
@@ -385,7 +386,8 @@ struct vrt_ctx {
unsigned syntax;
unsigned vclver;
unsigned method;
- unsigned *handling;
+
+ struct wrk_vpi *vpi;

/*
* msg is for error messages and exists only for
diff --git a/lib/libvcc/vcc_compile.c b/lib/libvcc/vcc_compile.c
index e0b065a8a..442f4f9ca 100644
--- a/lib/libvcc/vcc_compile.c
+++ b/lib/libvcc/vcc_compile.c
@@ -451,7 +451,7 @@ EmitInitFini(const struct vcc *tl)
if (VSB_len(p->ini))
Fc(tl, 0, "\t/* %u */\n%s\n", p->n, VSB_data(p->ini));
if (p->ignore_errors == 0) {
- Fc(tl, 0, "\tif (*ctx->handling == VCL_RET_FAIL)\n");
+ Fc(tl, 0, "\tif (ctx->vpi->handling == VCL_RET_FAIL)\n");
Fc(tl, 0, "\t\treturn(1);\n");
}
Fc(tl, 0, "\tvgc_inistep = %u;\n\n", p->n);
@@ -464,9 +464,9 @@ EmitInitFini(const struct vcc *tl)

/* Handle failures from vcl_init */
Fc(tl, 0, "\n");
- Fc(tl, 0, "\tif (*ctx->handling != VCL_RET_OK)\n");
+ Fc(tl, 0, "\tif (ctx->vpi->handling != VCL_RET_OK)\n");
Fc(tl, 0, "\t\treturn(1);\n");
- Fc(tl, 0, "\t*ctx->handling = 0;\n");
+ Fc(tl, 0, "\tctx->vpi->handling = 0;\n");

VTAILQ_FOREACH(sy, &tl->sym_objects, sideways) {
Fc(tl, 0, "\tif (!%s) {\n", sy->rname);
@@ -620,7 +620,7 @@ vcc_CompileSource(struct vcc *tl, struct source *sp, const char *jfile)
Fh(tl, 0, "/* ---===### VCC generated .h code ###===---*/\n");
Fc(tl, 0, "\n/* ---===### VCC generated .c code ###===---*/\n");

- Fc(tl, 0, "\n#define END_ if (*ctx->handling) return\n");
+ Fc(tl, 0, "\n#define END_ if (ctx->vpi->handling) return\n");

vcc_Parse_Init(tl);

diff --git a/vmod/vmod_debug.c b/vmod/vmod_debug.c
index d34d1f227..f8c603204 100644
--- a/vmod/vmod_debug.c
+++ b/vmod/vmod_debug.c
@@ -38,6 +38,8 @@

#include "cache/cache_varnishd.h"
#include "cache/cache_filter.h"
+#include "vcc_interface.h" // struct wrk_vpi
+

#include "vsa.h"
#include "vtim.h"
@@ -458,7 +460,7 @@ event_load(VRT_CTX, struct vmod_priv *priv)
// This should fail
AN(VRT_AddFilter(ctx, &xyzzy_vfp_rot13, &xyzzy_vdp_rot13));
// Reset the error, we know what we're doing.
- *ctx->handling = 0;
+ ctx->vpi->handling = 0;

VRT_RemoveFilter(ctx, &xyzzy_vfp_rot13, &xyzzy_vdp_rot13);
AZ(VRT_AddFilter(ctx, &xyzzy_vfp_rot13, &xyzzy_vdp_rot13));
diff --git a/vmod/vmod_debug_dyn.c b/vmod/vmod_debug_dyn.c
index 8ab467085..6b562cfc7 100644
--- a/vmod/vmod_debug_dyn.c
+++ b/vmod/vmod_debug_dyn.c
@@ -120,8 +120,7 @@ xyzzy_dyn__init(VRT_CTX, struct xyzzy_debug_dyn **dynp,
AN(vcl_name);

if (*addr == '\0' || *port == '\0') {
- AN(ctx->handling);
- AZ(*ctx->handling);
+ AZ(VRT_handled(ctx));
VRT_fail(ctx, "Missing dynamic backend address or port");
return;
}
_______________________________________________
varnish-commit mailing list
varnish-commit@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit