More cleanup and simplification of FetchError reporting.

bin/varnishd/cache.h

@@ -199,7 +199,6 @@ struct http_conn {
 	struct ws		*ws;
 	txt			rxbuf;
 	txt			pipeline;
-	const char		*error;
 };
 
 /*--------------------------------------------------------------------*/
@@ -785,7 +784,7 @@ void HTC_Init(struct http_conn *htc, struct ws *ws, int fd, unsigned vsl_id,
     unsigned maxbytes, unsigned maxhdr);
 int HTC_Reinit(struct http_conn *htc);
 int HTC_Rx(struct http_conn *htc);
-ssize_t HTC_Read(struct http_conn *htc, void *d, size_t len);
+ssize_t HTC_Read(struct worker *w, struct http_conn *htc, void *d, size_t len);
 int HTC_Complete(struct http_conn *htc);
 
 #define HTTPH(a, b, c, d, e, f, g) extern char b[];

bin/varnishd/cache_esi_fetch.c

@@ -44,7 +44,8 @@
  */
 
 static ssize_t
-vef_read(struct http_conn *htc, void *buf, ssize_t buflen, ssize_t bytes)
+vef_read(struct worker *w, struct http_conn *htc, void *buf, ssize_t buflen,
+    ssize_t bytes)
 {
 	ssize_t d;
 
@@ -55,7 +56,7 @@ vef_read(struct http_conn *htc, void *buf, ssize_t buflen, ssize_t bytes)
 		if (d < bytes)
 			bytes = d;
 	}
-	return (HTC_Read(htc, buf, bytes));
+	return (HTC_Read(w, htc, buf, bytes));
 }
 
 /*---------------------------------------------------------------------
@@ -74,7 +75,7 @@ vfp_esi_bytes_uu(struct worker *w, struct http_conn *htc, ssize_t bytes)
 		st = FetchStorage(w, 0);
 		if (st == NULL)
 			return (-1);
-		wl = vef_read(htc,
+		wl = vef_read(w, htc,
 		    st->ptr + st->len, st->space - st->len, bytes);
 		if (wl <= 0)
 			return (wl);
@@ -105,14 +106,14 @@ vfp_esi_bytes_gu(struct worker *w, struct http_conn *htc, ssize_t bytes)
 
 	while (bytes > 0) {
 		if (VGZ_IbufEmpty(vg) && bytes > 0) {
-			wl = vef_read(htc, ibuf, sizeof ibuf, bytes);
+			wl = vef_read(w, htc, ibuf, sizeof ibuf, bytes);
 			if (wl <= 0)
 				return (wl);
 			VGZ_Ibuf(vg, ibuf, wl);
 			bytes -= wl;
 		}
 		if (VGZ_ObufStorage(w, vg))
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gunzip(vg, &dp, &dl);
 		xxxassert(i == VGZ_OK || i == VGZ_END);
 		VEP_Parse(w, dp, dl);
@@ -180,7 +181,7 @@ vfp_vep_callback(struct worker *w, ssize_t l, enum vgz_flag flg)
 		}
 		do {
 			if (VGZ_ObufStorage(w, vef->vgz)) {
-				vef->error = errno;
+				vef->error = ENOMEM;
 				vef->tot += l;
 				return (vef->tot);
 			}
@@ -203,7 +204,7 @@ vfp_vep_callback(struct worker *w, ssize_t l, enum vgz_flag flg)
 }
 
 static int
-vfp_esi_bytes_ug(const struct worker *w, struct http_conn *htc, ssize_t bytes)
+vfp_esi_bytes_ug(struct worker *w, struct http_conn *htc, ssize_t bytes)
 {
 	ssize_t wl;
 	char ibuf[params->gzip_stack_buffer];
@@ -214,7 +215,7 @@ vfp_esi_bytes_ug(const struct worker *w, struct http_conn *htc, ssize_t bytes)
 	CHECK_OBJ_NOTNULL(vef, VEF_MAGIC);
 
 	while (bytes > 0) {
-		wl = vef_read(htc, ibuf, sizeof ibuf, bytes);
+		wl = vef_read(w, htc, ibuf, sizeof ibuf, bytes);
 		if (wl <= 0)
 			return (wl);
 		bytes -= wl;
@@ -240,7 +241,7 @@ vfp_esi_bytes_ug(const struct worker *w, struct http_conn *htc, ssize_t bytes)
  */
 
 static int 
-vfp_esi_bytes_gg(const struct worker *w, struct http_conn *htc, size_t bytes)
+vfp_esi_bytes_gg(struct worker *w, struct http_conn *htc, size_t bytes)
 {
 	ssize_t wl;
 	char ibuf[params->gzip_stack_buffer];
@@ -257,7 +258,7 @@ vfp_esi_bytes_gg(const struct worker *w, struct http_conn *htc, size_t bytes)
 	ibuf2[0] = 0; /* For Flexelint */
 
 	while (bytes > 0) {
-		wl = vef_read(htc, ibuf, sizeof ibuf, bytes);
+		wl = vef_read(w, htc, ibuf, sizeof ibuf, bytes);
 		if (wl <= 0)
 			return (wl);
 		bytes -= wl;
@@ -334,6 +335,7 @@ vfp_esi_bytes(struct worker *w, struct http_conn *htc, ssize_t bytes)
 	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
 	AZ(w->fetch_failed);
 	AN(w->vep);
+	assert(w->htc == htc);
 	if (w->is_gzip && w->do_gunzip)
 		i = vfp_esi_bytes_gu(w, htc, bytes);
 	else if (w->is_gunzip && w->do_gzip)

bin/varnishd/cache_fetch.c

@@ -120,14 +120,12 @@ vfp_nop_bytes(struct worker *w, struct http_conn *htc, ssize_t bytes)
 	while (bytes > 0) {
 		st = FetchStorage(w, 0);
 		if (st == NULL)
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		l = st->space - st->len;
 		if (l > bytes)
 			l = bytes;
-		wl = HTC_Read(htc, st->ptr + st->len, l);
-		if (wl < 0)
-			return(FetchError(w, htc->error));
-		if (wl == 0)
+		wl = HTC_Read(w, htc, st->ptr + st->len, l);
+		if (wl <= 0)
 			return (wl);
 		st->len += wl;
 		w->fetch_obj->len += wl;
@@ -173,7 +171,8 @@ static struct vfp vfp_nop = {
 };
 
 /*--------------------------------------------------------------------
- * Fetch Storage
+ * Fetch Storage to put object into.
+ *
  */
 
 struct storage *
@@ -196,7 +195,7 @@ FetchStorage(struct worker *w, ssize_t sz)
 		l = params->fetch_chunksize * 1024LL;
 	st = STV_alloc(w, l);
 	if (st == NULL) {
-		errno = ENOMEM;
+		(void)FetchError(w, "Could not get storage");
 		return (NULL);
 	}
 	AZ(st->len);
@@ -247,17 +246,11 @@ fetch_straight(struct worker *w, struct http_conn *htc, ssize_t cl)
 	return (0);
 }
 
-/*--------------------------------------------------------------------*/
-/* XXX: Cleanup.  It must be possible somehow :-( */
-
-#define CERR() do {						\
-		if (i != 1) {					\
-			WSLB(w, SLT_FetchError,			\
-			    "chunked read_error: %d (%s)",	\
-			    errno, htc->error);			\
-			return (-1);				\
-		}						\
-	} while (0)
+/*--------------------------------------------------------------------
+ * Read a chunked HTTP object.
+ * XXX: Reading one byte at a time is pretty pessimal.
+ */
+ 
 
 static int
 fetch_chunked(struct worker *w, struct http_conn *htc)
@@ -271,15 +264,17 @@ fetch_chunked(struct worker *w, struct http_conn *htc)
 	do {
 		/* Skip leading whitespace */
 		do {
-			i = HTC_Read(htc, buf, 1);
-			CERR();
+			i = HTC_Read(w, htc, buf, 1);
+			if (i <= 0)
+				return (i);
 		} while (vct_islws(buf[0]));
 
 		/* Collect hex digits, skipping leading zeros */
 		for (u = 1; u < sizeof buf; u++) {
 			do {
-				i = HTC_Read(htc, buf + u, 1);
-				CERR();
+				i = HTC_Read(w, htc, buf + u, 1);
+				if (i <= 0)
+					return (i);
 			} while (u == 1 && buf[0] == '0' && buf[u] == '0');
 			if (!vct_ishex(buf[u]))
 				break;
@@ -290,8 +285,9 @@ fetch_chunked(struct worker *w, struct http_conn *htc)
 
 		/* Skip trailing white space */
 		while(vct_islws(buf[u]) && buf[u] != '\n') {
-			i = HTC_Read(htc, buf + u, 1);
-			CERR();
+			i = HTC_Read(w, htc, buf + u, 1);
+			if (i <= 0)
+				return (i);
 		}
 
 		if (buf[u] != '\n') 
@@ -306,11 +302,13 @@ fetch_chunked(struct worker *w, struct http_conn *htc)
 			if (i <= 0)
 				return (-1);
 		}
-		i = HTC_Read(htc, buf, 1);
-		CERR();
+		i = HTC_Read(w, htc, buf, 1);
+		if (i <= 0)
+			return (-1);
 		if (buf[0] == '\r') {
-			i = HTC_Read(htc, buf, 1);
-			CERR();
+			i = HTC_Read(w, htc, buf, 1);
+			if (i <= 0)
+				return (-1);
 		}
 		if (buf[0] != '\n')
 			return (FetchError(w,"chunked tail no NL"));
@@ -358,7 +356,7 @@ FetchReqBody(struct sess *sp)
 				rdcnt = sizeof buf;
 			else
 				rdcnt = content_length;
-			rdcnt = HTC_Read(sp->htc, buf, rdcnt);
+			rdcnt = HTC_Read(sp->wrk, sp->htc, buf, rdcnt);
 			if (rdcnt <= 0)
 				return (1);
 			content_length -= rdcnt;
@@ -459,7 +457,7 @@ FetchHdr(struct sess *sp)
 
 	if (i < 0) {
 		WSP(sp, SLT_FetchError, "http first read error: %d %d (%s)",
-		    i, errno, w->htc->error);
+		    i, errno, strerror(errno));
 		VDI_CloseFd(sp->wrk);
 		/* XXX: other cleanup ? */
 		/* Retryable if we never received anything */
@@ -473,7 +471,7 @@ FetchHdr(struct sess *sp)
 		if (i < 0) {
 			WSP(sp, SLT_FetchError,
 			    "http first read error: %d %d (%s)",
-			    i, errno, w->htc->error);
+			    i, errno, strerror(errno));
 			VDI_CloseFd(sp->wrk);
 			/* XXX: other cleanup ? */
 			return (-1);

bin/varnishd/cache_gzip.c

@@ -480,17 +480,15 @@ vfp_gunzip_bytes(struct worker *w, struct http_conn *htc, ssize_t bytes)
 			l = sizeof ibuf;
 			if (l > bytes)
 				l = bytes;
-			wl = HTC_Read(htc, ibuf, l);
-			if (wl < 0)
-				return(FetchError(w, htc->error));
-			if (wl == 0)
+			wl = HTC_Read(w, htc, ibuf, l);
+			if (wl <= 0)
 				return (wl);
 			VGZ_Ibuf(vg, ibuf, wl);
 			bytes -= wl;
 		}
 
 		if (VGZ_ObufStorage(w, vg)) 
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gunzip(vg, &dp, &dl);
 		if (i != VGZ_OK && i != VGZ_END) 
 			return(FetchError(w, "Gunzip data error"));
@@ -560,16 +558,14 @@ vfp_gzip_bytes(struct worker *w, struct http_conn *htc, ssize_t bytes)
 			l = sizeof ibuf;
 			if (l > bytes)
 				l = bytes;
-			wl = HTC_Read(htc, ibuf, l);
-			if (wl < 0)
-				return(FetchError(w, htc->error));
-			if (wl == 0)
+			wl = HTC_Read(w, htc, ibuf, l);
+			if (wl <= 0)
 				return (wl);
 			VGZ_Ibuf(vg, ibuf, wl);
 			bytes -= wl;
 		}
 		if (VGZ_ObufStorage(w, vg)) 
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gzip(vg, &dp, &dl, VGZ_NORMAL);
 		assert(i == Z_OK);
 		w->fetch_obj->len += dl;
@@ -597,7 +593,7 @@ vfp_gzip_end(struct worker *w)
 	do {
 		VGZ_Ibuf(vg, "", 0);
 		if (VGZ_ObufStorage(w, vg))
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gzip(vg, &dp, &dl, VGZ_FINISH);
 		w->fetch_obj->len += dl;
 	} while (i != Z_STREAM_END);
@@ -648,14 +644,12 @@ vfp_testgzip_bytes(struct worker *w, struct http_conn *htc, ssize_t bytes)
 	while (bytes > 0) {
 		st = FetchStorage(w, 0);
 		if (st == NULL)
-			return(FetchError(w, "Could not get storage"));
+			return(-1);
 		l = st->space - st->len;
 		if (l > bytes)
 			l = bytes;
-		wl = HTC_Read(htc, st->ptr + st->len, l);
-		if (wl < 0)
-			return(FetchError(w, htc->error));
-		if (wl == 0)
+		wl = HTC_Read(w, htc, st->ptr + st->len, l);
+		if (wl <= 0)
 			return (wl);
 		bytes -= wl;
 		VGZ_Ibuf(vg, st->ptr + st->len, wl);

bin/varnishd/cache_httpconn.c

@@ -27,6 +27,17 @@
  * SUCH DAMAGE.
  *
  * HTTP protocol requests
+ *
+ * The trouble with the "until magic sequence" design of HTTP protocol messages
+ * is that either you have to read a single character at a time, which is
+ * inefficient, or you risk reading too much, and pre-read some of the object,
+ * or even the next pipelined request, which follows the one you want.
+ *
+ * HTC reads a HTTP protocol header into a workspace, subject to limits,
+ * and stops when we see the magic marker (double [CR]NL), and if we overshoot,
+ * it keeps track of the "pipelined" data.
+ *
+ * We use this both for client and backend connections.
  */
 
 #include "config.h"
@@ -53,7 +64,8 @@ htc_header_complete(txt *t)
 	/* Skip any leading white space */
 	for (p = t->b ; vct_issp(*p); p++)
 		continue;
-	if (*p == '\0') {
+	if (p == t->e) {
+		/* All white space */
 		t->e = t->b;
 		*t->e = '\0';
 		return (0);
@@ -85,7 +97,6 @@ HTC_Init(struct http_conn *htc, struct ws *ws, int fd, unsigned vsl_id,
 	htc->vsl_id = vsl_id;
 	htc->maxbytes = maxbytes;
 	htc->maxhdr = maxhdr;
-	htc->error = "No error recorded";
 
 	(void)WS_Reserve(htc->ws, htc->maxbytes);
 	htc->rxbuf.b = ws->f;
@@ -107,7 +118,6 @@ HTC_Reinit(struct http_conn *htc)
 	unsigned l;
 
 	CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
-	htc->error = "No error recorded";
 	(void)WS_Reserve(htc->ws, htc->maxbytes);
 	htc->rxbuf.b = htc->ws->f;
 	htc->rxbuf.e = htc->ws->f;
@@ -123,7 +133,7 @@ HTC_Reinit(struct http_conn *htc)
 }
 
 /*--------------------------------------------------------------------
- *
+ * Return 1 if we have a complete HTTP procol header
  */
 
 int
@@ -137,6 +147,8 @@ HTC_Complete(struct http_conn *htc)
 	if (i == 0)
 		return (0);
 	WS_ReleaseP(htc->ws, htc->rxbuf.e);
+	AZ(htc->pipeline.b);
+	AZ(htc->pipeline.e);
 	if (htc->rxbuf.b + i < htc->rxbuf.e) {
 		htc->pipeline.b = htc->rxbuf.b + i;
 		htc->pipeline.e = htc->rxbuf.e;
@@ -168,6 +180,10 @@ HTC_Rx(struct http_conn *htc)
 	}
 	i = read(htc->fd, htc->rxbuf.e, i);
 	if (i <= 0) {
+		/*
+		 * We wouldn't come here if we had a complete HTTP header
+		 * so consequently an EOF can not be OK
+		 */
 		WS_ReleaseP(htc->ws, htc->rxbuf.b);
 		return (-1);
 	}
@@ -176,16 +192,21 @@ HTC_Rx(struct http_conn *htc)
 	return (HTC_Complete(htc));
 }
 
+/*--------------------------------------------------------------------
+ * Read up to len bytes, returning pipelined data first.
+ */
+
 ssize_t
-HTC_Read(struct http_conn *htc, void *d, size_t len)
+HTC_Read(struct worker *w, struct http_conn *htc, void *d, size_t len)
 {
 	size_t l;
 	unsigned char *p;
 	ssize_t i;
 
+	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
+	CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
 	l = 0;
 	p = d;
-	CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
 	if (htc->pipeline.b) {
 		l = Tlen(htc->pipeline);
 		if (l > len)
@@ -201,9 +222,8 @@ HTC_Read(struct http_conn *htc, void *d, size_t len)
 		return (l);
 	i = read(htc->fd, p, len);
 	if (i < 0) {
-		htc->error = strerror(errno);
+		WSL(w, SLT_FetchError, htc->vsl_id, "%s", strerror(errno));
 		return (i);
-	} else if (i == 0)
-		htc->error = "Remote closed connection";
+	}
 	return (i + l);
 }