<div dir="ltr">Can we just do:<br><br> return (ctx->req->is_hit ? ctx->req->objcore->hits : 0);<br><br>in VRT_r_obj_hits() and revert this commit altogether?<br><br>I'm not sure whether this was discussed but besides the macros now having the wrong names (REQ_FLAG and VREQ[WR][01] used for other variables than req) adding more knobs is not the way to go IMO.<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 7, 2015 at 12:12 AM, Federico Schwindt <span dir="ltr"><<a href="mailto:fgsch@lodoss.net" target="_blank">fgsch@lodoss.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="im HOEnZb"><div dir="ltr">I must say I'm not very thrilled about having another variable to check whether it was a hit. <br><br>How much of an issue this really is and can we fix the code so we still use obj.hits?</div></span><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 7, 2015 at 12:10 AM, Federico Schwindt <span dir="ltr"><<a href="mailto:fgsch@lodoss.net" target="_blank">fgsch@lodoss.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I must say I'm not very thrilled about having another variable to check whether it was a hit. <br><br>How much of an issue this really is and can we fix the code so we still use obj.hits?<br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 6, 2015 at 9:41 PM, Nils Goroll <span dir="ltr"><<a href="mailto:nils.goroll@uplex.de" target="_blank">nils.goroll@uplex.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
commit 468a0472fa313dbff69269afc085957642011abc<br>
Author: Nils Goroll <<a href="mailto:nils.goroll@uplex.de" target="_blank">nils.goroll@uplex.de</a>><br>
Date: Tue Jan 6 22:24:21 2015 +0100<br>
<br>
Expose is_hit to VCL as resp.is_hit<br>
<br>
The common method to check obj.hits is racy: While the update to<br>
the hits counter happens under a lock, a hit could have happened<br>
before the missing request gets to read obj.hits.<br>
<br>
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h<br>
index 2e7fa47..82bf5ed 100644<br>
--- a/bin/varnishd/cache/cache.h<br>
+++ b/bin/varnishd/cache/cache.h<br>
@@ -540,7 +540,7 @@ struct req {<br>
int restarts;<br>
int esi_level;<br>
<br>
-#define REQ_FLAG(l, r, w, d) unsigned l:1;<br>
+#define REQ_FLAG(l, o, r, w, d) unsigned l:1;<br>
#include "tbl/req_flags.h"<br>
#undef REQ_FLAG<br>
<br>
diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c<br>
index d45458e..9bd99a7 100644<br>
--- a/bin/varnishd/cache/cache_panic.c<br>
+++ b/bin/varnishd/cache/cache_panic.c<br>
@@ -415,7 +415,7 @@ pan_req(const struct req *req)<br>
}<br>
<br>
VSB_printf(pan_vsp, " flags = {\n");<br>
-#define REQ_FLAG(l, r, w, d) if(req->l) VSB_printf(pan_vsp, " " #l ",\n");<br>
+#define REQ_FLAG(l, o, r, w, d) if(req->l) VSB_printf(pan_vsp, " " #l ",\n");<br>
#include "tbl/req_flags.h"<br>
#undef REQ_FLAG<br>
VSB_printf(pan_vsp, " }\n");<br>
diff --git a/bin/varnishd/cache/cache_vrt_var.c b/bin/varnishd/cache/cache_vrt_var.c<br>
index 4c6f2c6..5b2e5c2 100644<br>
--- a/bin/varnishd/cache/cache_vrt_var.c<br>
+++ b/bin/varnishd/cache/cache_vrt_var.c<br>
@@ -566,29 +566,29 @@ VRT_r_bereq_xid(VRT_CTX)<br>
* req fields<br>
*/<br>
<br>
-#define VREQW0(field)<br>
-#define VREQW1(field) \<br>
+#define VREQW0(obj, field)<br>
+#define VREQW1(obj, field) \<br>
void \<br>
-VRT_l_req_##field(VRT_CTX, unsigned a) \<br>
+VRT_l_##obj##_##field(VRT_CTX, unsigned a) \<br>
{ \<br>
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); \<br>
CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); \<br>
ctx->req->field = a ? 1 : 0; \<br>
}<br>
<br>
-#define VREQR0(field)<br>
-#define VREQR1(field) \<br>
+#define VREQR0(obj, field)<br>
+#define VREQR1(obj, field) \<br>
unsigned \<br>
-VRT_r_req_##field(VRT_CTX) \<br>
+VRT_r_##obj##_##field(VRT_CTX) \<br>
{ \<br>
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); \<br>
CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); \<br>
return (ctx->req->field); \<br>
}<br>
<br>
-#define REQ_FLAG(l, r, w, d) \<br>
- VREQR##r(l) \<br>
- VREQW##w(l)<br>
+#define REQ_FLAG(l, o, r, w, d) \<br>
+ VREQR##r(o, l) \<br>
+ VREQW##w(o, l)<br>
#include "tbl/req_flags.h"<br>
#undef REQ_FLAG<br>
<br>
diff --git a/bin/varnishtest/tests/v00013.vtc b/bin/varnishtest/tests/v00013.vtc<br>
index c428add..19e9f26 100644<br>
--- a/bin/varnishtest/tests/v00013.vtc<br>
+++ b/bin/varnishtest/tests/v00013.vtc<br>
@@ -13,6 +13,11 @@ varnish v1 -vcl+backend {<br>
<br>
sub vcl_deliver {<br>
set resp.http.foo = obj.hits;<br>
+ if (resp.is_hit) {<br>
+ set resp.http.X-Hit = "yes";<br>
+ } else {<br>
+ set resp.http.X-Hit = "no";<br>
+ }<br>
}<br>
} -start<br>
<br>
@@ -21,16 +26,19 @@ client c1 {<br>
rxresp<br>
expect resp.status == 200<br>
expect resp.http.foo == 0<br>
+ expect resp.http.X-hit == "no"<br>
<br>
txreq<br>
rxresp<br>
expect resp.status == 200<br>
expect resp.http.foo == 1<br>
+ expect resp.http.X-hit == "yes"<br>
<br>
txreq -url /foo<br>
rxresp<br>
expect resp.status == 200<br>
expect resp.http.foo == 0<br>
+ expect resp.http.X-hit == "no"<br>
delay .1<br>
<br>
txreq<br>
@@ -38,4 +46,5 @@ client c1 {<br>
expect resp.status == 200<br>
expect resp.http.foo == 2<br>
expect resp.http.bar >= 0.100<br>
+ expect resp.http.X-hit == "yes"<br>
} -run<br>
diff --git a/bin/varnishtest/tests/v00039.vtc b/bin/varnishtest/tests/v00039.vtc<br>
index 029e114..c248aa8 100644<br>
--- a/bin/varnishtest/tests/v00039.vtc<br>
+++ b/bin/varnishtest/tests/v00039.vtc<br>
@@ -13,6 +13,11 @@ varnish v1 \<br>
-vcl+backend {<br>
sub vcl_deliver {<br>
set resp.http.hits = obj.hits;<br>
+ if (resp.is_hit) {<br>
+ set resp.http.X-Hit = "yes";<br>
+ } else {<br>
+ set resp.http.X-Hit = "no";<br>
+ }<br>
}<br>
} -start<br>
<br>
@@ -23,6 +28,7 @@ client c1 {<br>
expect resp.status == 200<br>
expect resp.bodylen == 6<br>
expect resp.http.hits == 0<br>
+ expect resp.http.X-Hit == "no"<br>
<br>
# This is a hit -> hits == 1<br>
txreq -url "/" -hdr "Bar: 1"<br>
@@ -30,6 +36,7 @@ client c1 {<br>
expect resp.status == 200<br>
expect resp.bodylen == 6<br>
expect resp.http.hits == 1<br>
+ expect resp.http.X-Hit == "yes"<br>
<br>
# This is a miss on different vary -> hits == 0<br>
txreq -url "/" -hdr "Bar: 2"<br>
@@ -37,6 +44,7 @@ client c1 {<br>
expect resp.status == 200<br>
expect resp.bodylen == 4<br>
expect resp.http.hits == 0<br>
+ expect resp.http.X-Hit == "no"<br>
<br>
# This is a hit -> hits == 1<br>
txreq -url "/" -hdr "Bar: 2"<br>
@@ -44,6 +52,7 @@ client c1 {<br>
expect resp.status == 200<br>
expect resp.bodylen == 4<br>
expect resp.http.hits == 1<br>
+ expect resp.http.X-Hit == "yes"<br>
<br>
} -run<br>
<br>
diff --git a/include/tbl/req_flags.h b/include/tbl/req_flags.h<br>
index 6757d90..55f20f7 100644<br>
--- a/include/tbl/req_flags.h<br>
+++ b/include/tbl/req_flags.h<br>
@@ -29,11 +29,11 @@<br>
<br>
/*lint -save -e525 -e539 */<br>
<br>
-/* lower, vcl_r, vcl_w, doc */<br>
-REQ_FLAG(disable_esi, 0, 0, "")<br>
-REQ_FLAG(hash_ignore_busy, 1, 1, "")<br>
-REQ_FLAG(hash_always_miss, 1, 1, "")<br>
-REQ_FLAG(is_hit, 0, 0, "")<br>
-REQ_FLAG(wantbody, 0, 0, "")<br>
-REQ_FLAG(gzip_resp, 0, 0, "")<br>
+/* lower, vcl_obj, vcl_r, vcl_w, doc */<br>
+REQ_FLAG(disable_esi, none, 0, 0, "")<br>
+REQ_FLAG(hash_ignore_busy, req, 1, 1, "")<br>
+REQ_FLAG(hash_always_miss, req, 1, 1, "")<br>
+REQ_FLAG(is_hit, resp, 1, 0, "")<br>
+REQ_FLAG(wantbody, none, 0, 0, "")<br>
+REQ_FLAG(gzip_resp, none, 0, 0, "")<br>
/*lint -restore */<br>
diff --git a/lib/libvcc/generate.py b/lib/libvcc/generate.py<br>
index e582c0f..699bab3 100755<br>
--- a/lib/libvcc/generate.py<br>
+++ b/lib/libvcc/generate.py<br>
@@ -564,8 +564,10 @@ sp_variables = [<br>
'INT',<br>
( 'hit', 'deliver',),<br>
( ), """<br>
- The count of cache-hits on this object. A value of 0 indicates a<br>
- cache miss.<br>
+ The count of cache-hits on this object.<br>
+<br>
+ NB: obj.hits is not a reliable way to determine cache<br>
+ misses. See resp.is_hit.<br>
"""<br>
),<br>
('obj.http.',<br>
@@ -645,6 +647,13 @@ sp_variables = [<br>
The corresponding HTTP header.<br>
"""<br>
),<br>
+ ('resp.is_hit',<br>
+ 'BOOL',<br>
+ ( 'deliver',),<br>
+ ( ), """<br>
+ If this response is from a cache hit.<br>
+ """<br>
+ ),<br>
('now',<br>
'TIME',<br>
( 'all',),<br>
<br>
_______________________________________________<br>
varnish-commit mailing list<br>
<a href="mailto:varnish-commit@varnish-cache.org" target="_blank">varnish-commit@varnish-cache.org</a><br>
<a href="https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit" target="_blank">https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>