Hi, I had some time in my recent flights so I decided to give it a shot. As discussed at VDD15Q1, this changes the hash_data input to BLOB and adds automatic conversion from STRING to BLOB.
There are a few downsides with this approach: 1. Every time we call hash_data() on something else than a BLOB we'll perform the conversion. If we assume that most people will not hash the body (a fair assumption I believe) we're doing extra work that only favours a minority, penalising the rest. 2. Adds more pressure to the already tight workspace. I guess we could use something else here if required. The other thing that bothers me somewhat is the naming behind the BLOBs, vmod_priv. If BLOBs will become a first class citizen we should rename it to something that is not coupled to vmods. This is regardless of the direction we take. A better alternative would be to teach vcc to issue two different calls depending on the input, e.g. for BLOBs it will use VRT_hashblob(), for anything else use VRT_hashstring(), so point 1 and 2 only affect users of hash_data(req.body). Comments? OKs?
From 7b7340d117973a09cb58aedc9173194a8326266b Mon Sep 17 00:00:00 2001 From: "Federico G. Schwindt" <[email protected]> Date: Fri, 17 Apr 2015 14:25:32 +0100 Subject: [PATCH 1/2] Change hash_data() input type to BLOB Add support for BLOB conversion in vcc. This is a prerequisite to hash the request body once available. Discussed at VDD15Q1. --- bin/varnishd/cache/cache_hash.c | 14 ++++++++++++++ bin/varnishd/cache/cache_vrt.c | 30 ++++++++++++++++++------------ bin/varnishd/hash/hash_slinger.h | 2 ++ bin/varnishtest/tests/m00012.vtc | 2 +- bin/varnishtest/tests/r01693.vtc | 4 +--- bin/varnishtest/tests/v00018.vtc | 2 +- include/vrt.h | 7 ++++--- lib/libvcc/vcc_action.c | 2 +- lib/libvcc/vcc_expr.c | 22 +++++++++++++++++++--- 9 files changed, 61 insertions(+), 24 deletions(-) diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 8a15757..861679b 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -61,6 +61,7 @@ #include "hash/hash_slinger.h" #include "vsha256.h" #include "vtim.h" +#include "vrt.h" static const struct hash_slinger *hash; static struct objhead *private_oh; @@ -187,6 +188,19 @@ HSH_DeleteObjHead(struct worker *wrk, struct objhead *oh) } void +HSH_AddBlob(struct req *req, const struct vmod_priv *priv) +{ + + CHECK_OBJ_NOTNULL(req, REQ_MAGIC); + AN(req->sha256ctx); + if (priv != NULL) { + SHA256_Update(req->sha256ctx, priv->priv, priv->len); + VSLb(req->vsl, SLT_Hash, "%.*s", priv->len, priv->priv); + } else + SHA256_Update(req->sha256ctx, &priv, sizeof priv); +} + +void HSH_AddString(struct req *req, const char *str) { diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c index 63d2502..c7c2ce5 100644 --- a/bin/varnishd/cache/cache_vrt.c +++ b/bin/varnishd/cache/cache_vrt.c @@ -257,22 +257,12 @@ VRT_handling(VRT_CTX, unsigned hand) */ void -VRT_hashdata(VRT_CTX, const char *str, ...) +VRT_hashdata(VRT_CTX, const struct vmod_priv *priv) { - va_list ap; - const char *p; CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); - HSH_AddString(ctx->req, str); - va_start(ap, str); - while (1) { - p = va_arg(ap, const char *); - if (p == vrt_magic_string_end) - break; - HSH_AddString(ctx->req, p); - } - va_end(ap); + HSH_AddBlob(ctx->req, priv); /* * Add a 'field-separator' to make it more difficult to * manipulate the hash. @@ -358,6 +348,22 @@ VRT_BOOL_string(unsigned val) /*--------------------------------------------------------------------*/ +const struct vmod_priv * +VRT_STRING_blob(VRT_CTX, VCL_STRING s) +{ + struct vmod_priv *p; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + p = (void*)WS_Alloc(ctx->ws, sizeof *p); + AN(p); + memset(p, 0, sizeof *p); + p->len = strlen(s); + p->priv = WS_Copy(ctx->ws, s, -1); + return (p); +} + +/*--------------------------------------------------------------------*/ + void VRT_Rollback(VRT_CTX, const struct http *hp) { diff --git a/bin/varnishd/hash/hash_slinger.h b/bin/varnishd/hash/hash_slinger.h index 6f546ff..59acf3c 100644 --- a/bin/varnishd/hash/hash_slinger.h +++ b/bin/varnishd/hash/hash_slinger.h @@ -34,6 +34,7 @@ struct objcore; struct busyobj; struct worker; struct object; +struct vmod_priv; typedef void hash_init_f(int ac, char * const *av); typedef void hash_start_f(void); @@ -67,6 +68,7 @@ enum lookup_e HSH_Lookup(struct req *, struct objcore **, struct objcore **, int wait_for_busy, int always_insert); void HSH_Ref(struct objcore *o); void HSH_Init(const struct hash_slinger *slinger); +void HSH_AddBlob(struct req *, const struct vmod_priv *); void HSH_AddString(struct req *, const char *str); void HSH_Insert(struct worker *, const void *hash, struct objcore *); void HSH_Purge(struct worker *, struct objhead *, double ttl, double grace, diff --git a/bin/varnishtest/tests/m00012.vtc b/bin/varnishtest/tests/m00012.vtc index ef07f90..9233efc 100644 --- a/bin/varnishtest/tests/m00012.vtc +++ b/bin/varnishtest/tests/m00012.vtc @@ -21,7 +21,7 @@ client c1 { delay .1 -varnish v1 -errvcl {BLOBs can only be used as arguments to VMOD functions.} { +varnish v1 -errvcl {BLOBs can not be used in this context.} { backend b1 {.host = "127.0.0.1";} diff --git a/bin/varnishtest/tests/r01693.vtc b/bin/varnishtest/tests/r01693.vtc index f16d620..f6fdddd 100644 --- a/bin/varnishtest/tests/r01693.vtc +++ b/bin/varnishtest/tests/r01693.vtc @@ -12,9 +12,7 @@ varnish v1 -arg "-p vsl_mask=+Hash" -vcl+backend { } -start logexpect l1 -v v1 { - expect * 1001 Hash "1" - expect 0 1001 Hash "bar" - expect 0 1001 Hash "3" + expect * 1001 Hash "1bar3" expect 0 1001 Hash "/" expect 0 1001 Hash "127.0.0.1" } -start diff --git a/bin/varnishtest/tests/v00018.vtc b/bin/varnishtest/tests/v00018.vtc index aae3723..8604547 100644 --- a/bin/varnishtest/tests/v00018.vtc +++ b/bin/varnishtest/tests/v00018.vtc @@ -43,7 +43,7 @@ varnish v1 -errvcl {Expected ';' got 'if'} { sub vcl_recv { set req.url = "foo" if "bar"; } } -varnish v1 -errvcl {Symbol not found: 'req.hash' (expected type STRING_LIST):} { +varnish v1 -errvcl {Symbol not found: 'req.hash' (expected type BLOB):} { backend b { .host = "127.0.0.1"; } sub vcl_hash { hash_data(req.hash); } } diff --git a/include/vrt.h b/include/vrt.h index e034f91..ff54530 100644 --- a/include/vrt.h +++ b/include/vrt.h @@ -220,7 +220,7 @@ const char *VRT_GetHdr(VRT_CTX, const struct gethdr_s *); void VRT_SetHdr(VRT_CTX, const struct gethdr_s *, const char *, ...); void VRT_handling(VRT_CTX, unsigned hand); -void VRT_hashdata(VRT_CTX, const char *str, ...); +void VRT_hashdata(VRT_CTX, const struct vmod_priv *); /* Simple stuff */ int VRT_strcmp(const char *s1, const char *s2); @@ -247,7 +247,6 @@ int VRT_Vmod_Init(void **hdl, void *ptr, int len, const char *nm, const char *path, const char *file_id, VRT_CTX); void VRT_Vmod_Fini(void **hdl); -struct vmod_priv; typedef void vmod_priv_free_f(void *); struct vmod_priv { void *priv; @@ -264,8 +263,10 @@ struct vmod_priv *VRT_priv_top(VRT_CTX, void *vmod_id); /* Stevedore related functions */ int VRT_Stv(const char *nm); -/* Convert things to string */ +/* Convert things to blob */ +const struct vmod_priv *VRT_STRING_blob(VRT_CTX, VCL_STRING); +/* Convert things to string */ char *VRT_IP_string(VRT_CTX, VCL_IP); char *VRT_INT_string(VRT_CTX, VCL_INT); char *VRT_REAL_string(VRT_CTX, VCL_REAL); diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c index c4f984d..2fbd9ec 100644 --- a/lib/libvcc/vcc_action.c +++ b/lib/libvcc/vcc_action.c @@ -292,7 +292,7 @@ parse_hash_data(struct vcc *tl) SkipToken(tl, '('); Fb(tl, 1, "VRT_hashdata(ctx,\n "); - vcc_Expr(tl, STRING_LIST); + vcc_Expr(tl, BLOB); ERRCHK(tl); Fb(tl, 1, ");\n"); SkipToken(tl, ')'); diff --git a/lib/libvcc/vcc_expr.c b/lib/libvcc/vcc_expr.c index 6a03bff..3daf29e 100644 --- a/lib/libvcc/vcc_expr.c +++ b/lib/libvcc/vcc_expr.c @@ -427,9 +427,7 @@ vcc_expr_tostring(struct vcc *tl, struct expr **e, enum var_type fmt) break; case BLOB: VSB_printf(tl->sb, - "Wrong use of BLOB value.\n" - "BLOBs can only be used as arguments to VMOD" - " functions.\n"); + "BLOBs can not be used in this context.\n"); vcc_ErrWhere2(tl, (*e)->t1, tl->t); return; default: @@ -446,6 +444,21 @@ vcc_expr_tostring(struct vcc *tl, struct expr **e, enum var_type fmt) */ static void +vcc_expr_toblob(struct vcc *tl, struct expr **e, enum var_type fmt) +{ + CHECK_OBJ_NOTNULL(*e, EXPR_MAGIC); + AN(fmt == BLOB); + + if ((*e)->fmt != BLOB) { + vcc_expr_tostring(tl, e, STRING); + ERRCHK(tl); + *e = vcc_expr_edit(fmt, "VRT_STRING_blob(ctx, \v1)", *e, NULL); + } +} + +/*-------------------------------------------------------------------- + */ +static void vcc_Eval_Regsub(struct vcc *tl, struct expr **e, const struct symbol *sym) { struct expr *e2; @@ -1341,6 +1354,9 @@ vcc_Expr(struct vcc *tl, enum var_type fmt) if (fmt == STRING || fmt == STRING_LIST) { vcc_expr_tostring(tl, &e, fmt); ERRCHK(tl); + } else if (fmt == BLOB) { + vcc_expr_toblob(tl, &e, fmt); + ERRCHK(tl); } if (!tl->err && fmt != e->fmt) { VSB_printf(tl->sb, "Expression has type %s, expected %s\n", -- 2.1.4
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
