Attached.
OK?
On Wed, Jul 30, 2014 at 9:01 AM, Poul-Henning Kamp <[email protected]>
wrote:
> --------
> In message <CAJV_h0bR-h=i0e-GKwZrd5fE9q-1AsF=_sJ=gFJQ=
> [email protected]>
> , Federico Schwindt writes:
>
> Two things, on trivial-ish and the other serious-ish.
>
> The trivial thing is that I think we should put the actual
> rollback in VRT_Rollback() (give it an VCL_HTTP argument) so that
> std.rollback just wraps that.
>
> The other thing is that I'm not even close to being convinced that
> resetting the workspace is safe anymore, and I have no idea how I
> can convince myself that it is, in a world full of VMODs.
>
> We may have to give up on that, and only rollback the struct http.
>
> --
> Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
> [email protected] | TCP/IP since RFC 956
> FreeBSD committer | BSD since 4.3-tahoe
> Never attribute to malice what can adequately be explained by incompetence.
>
bin/varnishd/cache/cache.h | 1 +
bin/varnishd/cache/cache_fetch.c | 7 ++--
bin/varnishd/cache/cache_vrt.c | 17 +++++++---
bin/varnishtest/tests/m00017.vtc | 69 ++++++++++++++++++++++++++++++++++++++++
bin/varnishtest/tests/r01512.vtc | 56 ++++++++++++++++++++++++++++++++
doc/sphinx/reference/vcl.rst | 3 +-
include/vrt.h | 2 +-
lib/libvcc/vcc_action.c | 2 +-
lib/libvmod_std/vmod.vcc | 7 ++++
lib/libvmod_std/vmod_std.c | 6 ++++
10 files changed, 160 insertions(+), 10 deletions(-)
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index f21c57d..e171d66 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -490,6 +490,7 @@ struct busyobj {
enum busyobj_state_e state;
struct ws ws[1];
+ char *ws_bo;
struct vbc *vbc;
struct http *bereq0;
struct http *bereq;
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 1e45392..70f467a 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -220,6 +220,10 @@ vbf_stp_mkbereq(const struct worker *wrk, struct busyobj *bo)
}
}
+ HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
+ bo->ws_bo = WS_Snapshot(bo->ws);
+ HTTP_Copy(bo->bereq, bo->bereq0);
+
VBO_setstate(bo, BOS_REQ_DONE);
return (F_STP_STARTFETCH);
}
@@ -267,9 +271,6 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
else
AZ(bo->req);
- HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
- HTTP_Copy(bo->bereq, bo->bereq0);
-
http_PrintfHeader(bo->bereq, "X-Varnish: %u", VXID(bo->vsl->wid));
VCL_backend_fetch_method(bo->vcl, wrk, NULL, bo, bo->bereq->ws);
diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 4038693..edfa2a5 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -357,13 +357,22 @@ VRT_BOOL_string(unsigned val)
/*--------------------------------------------------------------------*/
void
-VRT_Rollback(const struct vrt_ctx *ctx)
+VRT_Rollback(const struct vrt_ctx *ctx, const struct http *hp)
{
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
- HTTP_Copy(ctx->req->http, ctx->req->http0);
- WS_Reset(ctx->req->ws, ctx->req->ws_req);
+ CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
+ if (hp == ctx->http_req) {
+ CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
+ HTTP_Copy(ctx->req->http, ctx->req->http0);
+ WS_Reset(ctx->req->ws, ctx->req->ws_req);
+ } else if (hp == ctx->http_bereq) {
+ CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
+ HTTP_Copy(ctx->bo->bereq, ctx->bo->bereq0);
+ WS_Reset(ctx->bo->bereq->ws, ctx->bo->ws_bo);
+ WS_Reset(ctx->bo->ws, ctx->bo->ws_bo);
+ } else
+ WRONG("VRT_Rollback 'hp' invalid");
}
/*--------------------------------------------------------------------*/
diff --git a/bin/varnishtest/tests/m00017.vtc b/bin/varnishtest/tests/m00017.vtc
new file mode 100644
index 0000000..5622ec0
--- /dev/null
+++ b/bin/varnishtest/tests/m00017.vtc
@@ -0,0 +1,69 @@
+varnishtest "Test std.rollback"
+
+server s1 {
+ rxreq
+ expect req.url == "/foo"
+ expect req.http.foobar == "bar"
+ txresp -status 400
+ accept
+ rxreq
+ expect req.url == "/bar"
+ expect req.http.foobar == "foo"
+ txresp
+ accept
+ rxreq
+ expect req.url == "/baz"
+ expect req.http.foobar == "qux"
+ txresp -status 400
+ accept
+ rxreq
+ expect req.url == "/qux"
+ expect req.http.foobar == "baz"
+ txresp
+} -start
+
+varnish v1 -vcl+backend {
+ import ${vmod_std};
+
+ sub vcl_recv {
+ if (req.url == "/foo") {
+ set req.http.foobar = "bar";
+ }
+ }
+
+ sub vcl_deliver {
+ if (resp.status == 400) {
+ std.rollback(req);
+ set req.url = "/bar";
+ return (restart);
+ }
+ }
+} -start
+
+client c1 {
+ txreq -url "/foo" -hdr "foobar: foo"
+ rxresp
+} -run
+
+varnish v1 -vcl+backend {
+ import ${vmod_std};
+
+ sub vcl_backend_fetch {
+ if (bereq.url == "/baz") {
+ set bereq.http.foobar = "qux";
+ }
+ }
+
+ sub vcl_backend_response {
+ if (beresp.status == 400) {
+ std.rollback(bereq);
+ set bereq.url = "/qux";
+ return (retry);
+ }
+ }
+}
+
+client c1 {
+ txreq -url "/baz" -hdr "foobar: baz"
+ rxresp
+} -run
diff --git a/bin/varnishtest/tests/r01512.vtc b/bin/varnishtest/tests/r01512.vtc
new file mode 100644
index 0000000..71496a2
--- /dev/null
+++ b/bin/varnishtest/tests/r01512.vtc
@@ -0,0 +1,56 @@
+varnishtest "Regression test for #1512"
+
+# First test bereq changes across v_b_r and v_b_f
+
+server s1 {
+ rxreq
+ txresp -status 700
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_backend_fetch {
+ if (bereq.http.x-abandon == "1") {
+ return (abandon);
+ }
+ }
+ sub vcl_backend_response {
+ if (beresp.status == 700) {
+ set bereq.http.x-abandon = "1";
+ return (retry);
+ }
+ }
+ sub vcl_synth {
+ set resp.status = 701;
+ }
+} -start
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 701
+} -run
+
+# Then across v_b_e and v_b_f
+
+varnish v1 -vcl {
+ backend bad { .host = "${bad_ip}"; }
+ sub vcl_backend_fetch {
+ set bereq.backend = bad;
+ if (bereq.http.x-abandon == "2") {
+ return (abandon);
+ }
+ }
+ sub vcl_backend_error {
+ set bereq.http.x-abandon = "2";
+ return (retry);
+ }
+ sub vcl_synth {
+ set resp.status = 702;
+ }
+}
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 702
+} -run
diff --git a/doc/sphinx/reference/vcl.rst b/doc/sphinx/reference/vcl.rst
index 743689e..6828f99 100644
--- a/doc/sphinx/reference/vcl.rst
+++ b/doc/sphinx/reference/vcl.rst
@@ -369,7 +369,8 @@ return()
in the request handling state machine.
rollback()
- Restore request HTTP headers to their original state.
+ Restore *req* HTTP headers to their original state. This function is
+ deprecated. Use std.rollback() instead.
synthetic(STRING)
Prepare a synthetic response body containing the STRING. Available in
diff --git a/include/vrt.h b/include/vrt.h
index efc9c94..ead55d3 100644
--- a/include/vrt.h
+++ b/include/vrt.h
@@ -249,7 +249,7 @@ void VRT_hashdata(const struct vrt_ctx *, const char *str, ...);
int VRT_strcmp(const char *s1, const char *s2);
void VRT_memmove(void *dst, const void *src, unsigned len);
-void VRT_Rollback(const struct vrt_ctx *);
+void VRT_Rollback(const struct vrt_ctx *, const struct http *);
/* Synthetic pages */
void VRT_synth_page(const struct vrt_ctx *, const char *, ...);
diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c
index 9b1d22d..1b0c4b2 100644
--- a/lib/libvcc/vcc_action.c
+++ b/lib/libvcc/vcc_action.c
@@ -376,7 +376,7 @@ parse_rollback(struct vcc *tl)
{
vcc_NextToken(tl);
- Fb(tl, 1, "VRT_Rollback(ctx);\n");
+ Fb(tl, 1, "VRT_Rollback(ctx, VRT_r_req(ctx));\n");
}
/*--------------------------------------------------------------------*/
diff --git a/lib/libvmod_std/vmod.vcc b/lib/libvmod_std/vmod.vcc
index fbee007..5bcc403 100644
--- a/lib/libvmod_std/vmod.vcc
+++ b/lib/libvmod_std/vmod.vcc
@@ -174,6 +174,13 @@ $Function INT port(IP)
Description
Returns the port number of an IP address.
+$Function VOID rollback(HTTP)
+
+Description
+ Restore *h* HTTP headers to their original state.
+Example
+ std.rollback(bereq);
+
$Function VOID timestamp(STRING)
Description
diff --git a/lib/libvmod_std/vmod_std.c b/lib/libvmod_std/vmod_std.c
index 718fb71..c5d0cfe 100644
--- a/lib/libvmod_std/vmod_std.c
+++ b/lib/libvmod_std/vmod_std.c
@@ -205,6 +205,12 @@ vmod_port(const struct vrt_ctx *ctx, VCL_IP ip)
return (VSA_Port(ip));
}
+VCL_VOID __match_proto__(td_std_rollback)
+vmod_rollback(const struct vrt_ctx *ctx, VCL_HTTP hp)
+{
+ VRT_Rollback(ctx, hp);
+}
+
VCL_VOID __match_proto__(td_std_timestamp)
vmod_timestamp(const struct vrt_ctx *ctx, VCL_STRING label)
{
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
