More cleanup and simplification of FetchError reporting.

Conflicts:

	bin/varnishd/cache_esi_fetch.c
	bin/varnishd/cache_fetch.c
	bin/varnishd/cache_gzip.c

bin/varnishd/cache.h

@@ -203,7 +203,6 @@ struct http_conn {
 	struct ws		*ws;
 	txt			rxbuf;
 	txt			pipeline;
-	const char		*error;
 };
 
 /*--------------------------------------------------------------------*/
@@ -719,8 +718,8 @@ int EXP_NukeOne(struct worker *w, struct lru *lru);
 
 /* cache_fetch.c */
 struct storage *FetchStorage(const struct sess *sp, ssize_t sz);
-int FetchError(struct sess *sp, const char *error);
-int FetchError2(struct sess *sp, const char *error, const char *more);
+int FetchError(const struct sess *sp, const char *error);
+int FetchError2(const struct sess *sp, const char *error, const char *more);
 int FetchHdr(struct sess *sp);
 int FetchBody(struct sess *sp);
 int FetchReqBody(struct sess *sp);
@@ -798,7 +797,7 @@ void HTC_Init(struct http_conn *htc, struct ws *ws, int fd, 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

@@ -46,7 +46,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;
 
@@ -57,7 +58,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));
 }
 
 /*---------------------------------------------------------------------
@@ -76,7 +77,7 @@ vfp_esi_bytes_uu(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 		st = FetchStorage(sp, 0);
 		if (st == NULL)
 			return (-1);
-		w = vef_read(htc,
+		w = vef_read(sp->wrk, htc,
 		    st->ptr + st->len, st->space - st->len, bytes);
 		if (w <= 0)
 			return (w);
@@ -107,14 +108,14 @@ vfp_esi_bytes_gu(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 
 	while (bytes > 0) {
 		if (VGZ_IbufEmpty(vg) && bytes > 0) {
-			w = vef_read(htc, ibuf, sizeof ibuf, bytes);
+			w = vef_read(sp->wrk, htc, ibuf, sizeof ibuf, bytes);
 			if (w <= 0)
 				return (w);
 			VGZ_Ibuf(vg, ibuf, w);
 			bytes -= w;
 		}
 		if (VGZ_ObufStorage(sp, vg))
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gunzip(vg, &dp, &dl);
 		xxxassert(i == VGZ_OK || i == VGZ_END);
 		VEP_parse(sp, dp, dl);
@@ -182,7 +183,7 @@ vfp_vep_callback(const struct sess *sp, ssize_t l, enum vgz_flag flg)
 		}
 		do {
 			if (VGZ_ObufStorage(sp, vef->vgz)) {
-				vef->error = errno;
+				vef->error = ENOMEM;
 				vef->tot += l;
 				return (vef->tot);
 			}
@@ -216,7 +217,7 @@ vfp_esi_bytes_ug(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 	CHECK_OBJ_NOTNULL(vef, VEF_MAGIC);
 
 	while (bytes > 0) {
-		w = vef_read(htc, ibuf, sizeof ibuf, bytes);
+		w = vef_read(sp->wrk, htc, ibuf, sizeof ibuf, bytes);
 		if (w <= 0)
 			return (w);
 		bytes -= w;
@@ -259,7 +260,7 @@ vfp_esi_bytes_gg(struct sess *sp, struct http_conn *htc, size_t bytes)
 	ibuf2[0] = 0; /* For Flexelint */
 
 	while (bytes > 0) {
-		w = vef_read(htc, ibuf, sizeof ibuf, bytes);
+		w = vef_read(sp->wrk, htc, ibuf, sizeof ibuf, bytes);
 		if (w <= 0)
 			return (w);
 		bytes -= w;
@@ -336,6 +337,7 @@ vfp_esi_bytes(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	AZ(sp->wrk->fetch_failed);
 	AN(sp->wrk->vep);
+	assert(sp->wrk->htc == htc);
 	if (sp->wrk->is_gzip && sp->wrk->do_gunzip)
 		i = vfp_esi_bytes_gu(sp, htc, bytes);
 	else if (sp->wrk->is_gunzip && sp->wrk->do_gzip)

bin/varnishd/cache_fetch.c

@@ -52,7 +52,7 @@ static unsigned fetchfrag;
  */
 
 int
-FetchError2(struct sess *sp, const char *error, const char *more)
+FetchError2(const struct sess *sp, const char *error, const char *more)
 {
 	struct worker *w;
 
@@ -71,7 +71,7 @@ FetchError2(struct sess *sp, const char *error, const char *more)
 }
 
 int
-FetchError(struct sess *sp, const char *error)
+FetchError(const struct sess *sp, const char *error)
 {
 	return(FetchError2(sp, error, NULL));
 }
@@ -125,15 +125,13 @@ vfp_nop_bytes(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 	while (bytes > 0) {
 		st = FetchStorage(sp, 0);
 		if (st == NULL)
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		l = st->space - st->len;
 		if (l > bytes)
 			l = bytes;
-		w = HTC_Read(htc, st->ptr + st->len, l);
-		if (w < 0)
-			return(FetchError(sp, htc->error));
-		if (w == 0)
-			return (w);
+		w = HTC_Read(sp->wrk, htc, st->ptr + st->len, l);
+		if (w <= 0)
+			return(w);
 		st->len += w;
 		sp->obj->len += w;
 		bytes -= w;
@@ -178,7 +176,8 @@ static struct vfp vfp_nop = {
 };
 
 /*--------------------------------------------------------------------
- * Fetch Storage
+ * Fetch Storage to put object into.
+ *
  */
 
 struct storage *
@@ -198,7 +197,7 @@ FetchStorage(const struct sess *sp, ssize_t sz)
 		l = params->fetch_chunksize * 1024LL;
 	st = STV_alloc(sp, l);
 	if (st == NULL) {
-		errno = ENOMEM;
+		(void)FetchError(sp, "Could not get storage");
 		return (NULL);
 	}
 	AZ(st->len);
@@ -250,17 +249,11 @@ fetch_straight(struct sess *sp, struct http_conn *htc, ssize_t cl)
 	return (0);
 }
 
-/*--------------------------------------------------------------------*/
-/* XXX: Cleanup.  It must be possible somehow :-( */
-
-#define CERR() do {						\
-		if (i != 1) {					\
-			WSP(sp, 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 sess *sp, struct http_conn *htc)
@@ -274,15 +267,17 @@ fetch_chunked(struct sess *sp, struct http_conn *htc)
 	do {
 		/* Skip leading whitespace */
 		do {
-			i = HTC_Read(htc, buf, 1);
-			CERR();
+			i = HTC_Read(sp->wrk, 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(sp->wrk, htc, buf + u, 1);
+				if (i <= 0)
+					return (i);
 			} while (u == 1 && buf[0] == '0' && buf[u] == '0');
 			if (!vct_ishex(buf[u]))
 				break;
@@ -294,8 +289,9 @@ fetch_chunked(struct sess *sp, 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(sp->wrk, htc, buf + u, 1);
+			if (i <= 0)
+				return (i);
 		}
 
 		if (buf[u] != '\n') {
@@ -308,13 +304,16 @@ fetch_chunked(struct sess *sp, struct http_conn *htc)
 			return (FetchError(sp,"chunked header number syntax"));
 		} else if (cl > 0) {
 			i = sp->wrk->vfp->bytes(sp, htc, cl);
-			CERR();
+			if (i <= 0)
+				return (-1);
 		}
-		i = HTC_Read(htc, buf, 1);
-		CERR();
+		i = HTC_Read(sp->wrk, htc, buf, 1);
+		if (i <= 0)
+			return (-1);
 		if (buf[0] == '\r') {
-			i = HTC_Read(htc, buf, 1);
-			CERR();
+			i = HTC_Read(sp->wrk, htc, buf, 1);
+			if (i <= 0)
+				return (-1);
 		}
 		if (buf[0] != '\n')
 			return (FetchError(sp,"chunked tail no NL"));
@@ -362,7 +361,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;
@@ -463,7 +462,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);
 		/* XXX: other cleanup ? */
 		/* Retryable if we never received anything */
@@ -477,7 +476,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);
 			/* XXX: other cleanup ? */
 			return (-1);

bin/varnishd/cache_gzip.c

@@ -474,17 +474,15 @@ vfp_gunzip_bytes(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 			l = sizeof ibuf;
 			if (l > bytes)
 				l = bytes;
-			w = HTC_Read(htc, ibuf, l);
-			if (w < 0)
-				return(FetchError(sp, htc->error));
-			if (w == 0)
+			w = HTC_Read(sp->wrk, htc, ibuf, l);
+			if (w <= 0)
 				return (w);
 			VGZ_Ibuf(vg, ibuf, w);
 			bytes -= w;
 		}
 
 		if (VGZ_ObufStorage(sp, vg))
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gunzip(vg, &dp, &dl);
 		if (i != VGZ_OK && i != VGZ_END)
 			return(FetchError(sp, "Gunzip data error"));
@@ -554,16 +552,14 @@ vfp_gzip_bytes(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 			l = sizeof ibuf;
 			if (l > bytes)
 				l = bytes;
-			w = HTC_Read(htc, ibuf, l);
-			if (w < 0)
-				return(FetchError(sp, htc->error));
-			if (w == 0)
+			w = HTC_Read(sp->wrk, htc, ibuf, l);
+			if (w <= 0)
 				return (w);
 			VGZ_Ibuf(vg, ibuf, w);
 			bytes -= w;
 		}
 		if (VGZ_ObufStorage(sp, vg))
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gzip(vg, &dp, &dl, VGZ_NORMAL);
 		assert(i == Z_OK);
 		sp->obj->len += dl;
@@ -592,7 +588,7 @@ vfp_gzip_end(struct sess *sp)
 	do {
 		VGZ_Ibuf(vg, "", 0);
 		if (VGZ_ObufStorage(sp, vg))
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		i = VGZ_Gzip(vg, &dp, &dl, VGZ_FINISH);
 		sp->obj->len += dl;
 	} while (i != Z_STREAM_END);
@@ -643,14 +639,12 @@ vfp_testgzip_bytes(struct sess *sp, struct http_conn *htc, ssize_t bytes)
 	while (bytes > 0) {
 		st = FetchStorage(sp, 0);
 		if (st == NULL)
-			return(FetchError(sp, "Could not get storage"));
+			return(-1);
 		l = st->space - st->len;
 		if (l > bytes)
 			l = bytes;
-		w = HTC_Read(htc, st->ptr + st->len, l);
-		if (w < 0)
-			return(FetchError(sp, htc->error));
-		if (w == 0)
+		w = HTC_Read(sp->wrk, htc, st->ptr + st->len, l);
+		if (w <= 0)
 			return (w);
 		bytes -= w;
 		VGZ_Ibuf(vg, st->ptr + st->len, w);

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"
@@ -57,7 +68,8 @@ htc_header_complete(txt *t)
 	/* Skip any leading white space */
 	for (p = t->b ; isspace(*p); p++)
 		continue;
-	if (*p == '\0') {
+	if (p == t->e) {
+		/* All white space */
 		t->e = t->b;
 		*t->e = '\0';
 		return (0);
@@ -88,7 +100,6 @@ HTC_Init(struct http_conn *htc, struct ws *ws, int fd, unsigned maxbytes,
 	htc->fd = fd;
 	htc->maxbytes = maxbytes;
 	htc->maxhdr = maxhdr;
-	htc->error = "No error recorded";
 
 	(void)WS_Reserve(htc->ws, htc->maxbytes);
 	htc->rxbuf.b = ws->f;
@@ -110,7 +121,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;
@@ -126,7 +136,7 @@ HTC_Reinit(struct http_conn *htc)
 }
 
 /*--------------------------------------------------------------------
- *
+ * Return 1 if we have a complete HTTP procol header
  */
 
 int
@@ -140,6 +150,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;
@@ -171,6 +183,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);
 	}
@@ -179,16 +195,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)
@@ -204,9 +225,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->fd, "%s", strerror(errno));
 		return (i);
-	} else if (i == 0)
-		htc->error = "Remote closed connection";
+	}
 	return (i + l);
 }