[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[Libevent-users] ev_send_error() and friends

Forwarding this discussion from Bug #3006553 to the ML:

First of all, I've attached a new version of my patch to this Mail -
this one just allows NULL reasons for evhttp_response_code and changes
evhttp_send_error to display the actual reason. I did not however change
the parameter semantics from "http status phrase" to "arbitrary
message" (as in the first patch).

This is about the only thing I can do without changing interfaces or
semantics. I'd consider it a fix for the "evhttp_send_error always
displays HTTP error 405 body" bug.

Oh, and I removed the redundant "Connection: close" code which later
happens in evhttp_send_page().

If you have no objections, feel free to apply it.

With that issue resolved there are other problems however:

1. The reason parameter is now redundant, everywhere, at all send
routines. You should probably always set it to NULL, both for
semi-correctness and to save some bytes of space (it will still do the
same thing as before if you give it a value though).

That's unfortunate but unavoidable if you want to keep compatibility.

2. Having a function like evhttp_send_error() with the new behaviour
just asks people to abuse the HTTP status phrase for detailed error
messages (in fact your documentation says that this is what it is
for...).

It should therefore be deprecated and supplemented by another function -
I'm calling it evhttp_send_reply_html() but you might have a better
name.

That's what the second patch is.

I included an implementation of said function and made the
404 handler use it. I'd prefer it if evhttp_send_reply_html() would
html-escape itself, but doing that properly would probably mean going
through buffer filters?

> Renaming the function or changing its interface in a
> backward-incompatible is NOT an option; we try very hard not to break
> existing code. 

I sympathize, seriously...

> Refactoring redundant code or simplifying the current thing would be
> cool (though it might well stand to wait for Libevent 2.1.x), as would
> having evhttp_send_error do the right thing when message/reason is
> NULL. I like your current patch fine, though.

I think both (new) patches are pretty harmless. Would be nice if you
could somehow get rid of all the "reason" parameters though...

> (Other than that, what do you mean by "implement
> evbuffer_add_vprintf"? It already exists.)

Oh. Right. Saw it, using it. :-)

Cheers,

Felix

P.S.: It's 2:15 AM now and although I tested my code I obviously can't
be sure that I didn't break anything. Also when doing "make verify"
regress always just says "FAILED" to me... But it did that even before I
changed anything.

diff --git a/http.c b/http.c
index c4fb09c..b9666a8 100644
--- a/http.c
+++ b/http.c
@@ -171,6 +171,7 @@ static void evhttp_read_header(struct evhttp_connection *evcon,
     struct evhttp_request *req);
 static int evhttp_add_header_internal(struct evkeyvalq *headers,
     const char *key, const char *value);
+static const char *evhttp_response_phrase_internal(int code);
 
 /* callbacks for bufferevent */
 static void evhttp_read_cb(struct bufferevent *, void *);
@@ -2030,11 +2031,15 @@ evhttp_send_done(struct evhttp_connection *evcon, void *arg)
 void
 evhttp_send_error(struct evhttp_request *req, int error, const char *reason)
 {
+	if(reason == NULL)
+	{
+		reason = evhttp_response_phrase_internal(error);
+	}
+
 #define ERR_FORMAT "<HTML><HEAD>\n" \
 	    "<TITLE>%d %s</TITLE>\n" \
 	    "</HEAD><BODY>\n" \
-	    "<H1>Method Not Implemented</H1>\n" \
-	    "Invalid method in request<P>\n" \
+	    "<H1>%s</H1>\n" \
 	    "</BODY></HTML>\n"
 
 	struct evbuffer *buf = evbuffer_new();
@@ -2044,12 +2049,9 @@ evhttp_send_error(struct evhttp_request *req, int error, const char *reason)
 		return;
 	}
 
-	/* close the connection on error */
-	evhttp_add_header(req->output_headers, "Connection", "close");
-
 	evhttp_response_code(req, error, reason);
 
-	evbuffer_add_printf(buf, ERR_FORMAT, error, reason);
+	evbuffer_add_printf(buf, ERR_FORMAT, error, reason, reason);
 
 	evhttp_send_page(req, buf);
 
@@ -2168,6 +2170,96 @@ evhttp_send_reply_end(struct evhttp_request *req)
 	}
 }
 
+static const char *informational_phrases[] = {
+	/* 100 */ "Continue",
+	/* 101 */ "Switching Protocols"
+};
+
+static const char *success_phrases[] = {
+	/* 200 */ "OK",
+	/* 201 */ "Created",
+	/* 202 */ "Accepted",
+	/* 203 */ "Non-Authoritative Information",
+	/* 204 */ "No Content",
+	/* 205 */ "Reset Content",
+	/* 206 */ "Partial Content"
+};
+
+static const char *redirection_phrases[] = {
+	/* 300 */ "Multiple Choices",
+	/* 301 */ "Moved Permanently",
+	/* 302 */ "Found",
+	/* 303 */ "See Other",
+	/* 304 */ "Not Modified",
+	/* 305 */ "Use Proxy",
+	/* 307 */ "Temporary Redirect"
+};
+
+static const char *client_error_phrases[] = {
+	/* 400 */ "Bad Request",
+	/* 401 */ "Unauthorized",
+	/* 402 */ "Payment Required",
+	/* 403 */ "Forbidden",
+	/* 404 */ "Not Found",
+	/* 405 */ "Method Not Allowed",
+	/* 406 */ "Not Acceptable",
+	/* 407 */ "Proxy Authentication Required",
+	/* 408 */ "Request Time-out",
+	/* 409 */ "Conflict",
+	/* 410 */ "Gone",
+	/* 411 */ "Length Required",
+	/* 412 */ "Precondition Failed",
+	/* 413 */ "Request Entity Too Large",
+	/* 414 */ "Request-URI Too Large",
+	/* 415 */ "Unsupported Media Type",
+	/* 416 */ "Requested range not satisfiable",
+	/* 417 */ "Expectation Failed"
+};
+
+static const char *server_error_phrases[] = {
+	/* 500 */ "Internal Server Error",
+	/* 501 */ "Not Implemented",
+	/* 502 */ "Bad Gateway",
+	/* 503 */ "Service Unavailable",
+	/* 504 */ "Gateway Time-out",
+	/* 505 */ "HTTP Version not supported"
+};
+
+struct response_class {
+	const char *name;
+	size_t num_responses;
+	const char **responses;
+};
+
+#ifndef MEMBERSOF
+#define MEMBERSOF(x) (sizeof(x)/sizeof(x[0]))
+#endif
+
+static const struct response_class response_classes[] = {
+	/* 1xx */ { "Informational", MEMBERSOF(informational_phrases), informational_phrases },
+	/* 2xx */ { "Success", MEMBERSOF(success_phrases), success_phrases },
+	/* 3xx */ { "Redirection", MEMBERSOF(redirection_phrases), redirection_phrases },
+	/* 4xx */ { "Client Error", MEMBERSOF(client_error_phrases), client_error_phrases },
+	/* 5xx */ { "Server Error", MEMBERSOF(server_error_phrases), server_error_phrases }
+};
+
+static const char *
+evhttp_response_phrase_internal(int code)
+{
+	int klass = code / 100 - 1;
+	int subcode = code % 100;
+
+	/* Unknown class - can't do any better here */
+	if (klass < 0 || klass >= MEMBERSOF(response_classes))
+		return "Unknown Status Class";
+
+	/* Unknown sub-code, return class name at least */
+	if (subcode >= response_classes[klass].num_responses)
+		return response_classes[klass].name;
+
+	return response_classes[klass].responses[subcode];
+}
+
 void
 evhttp_response_code(struct evhttp_request *req, int code, const char *reason)
 {
@@ -2175,6 +2267,8 @@ evhttp_response_code(struct evhttp_request *req, int code, const char *reason)
 	req->response_code = code;
 	if (req->response_code_line != NULL)
 		mm_free(req->response_code_line);
+	if (reason == NULL)
+		reason = evhttp_response_phrase_internal(code);
 	req->response_code_line = mm_strdup(reason);
 }
 

diff --git a/http.c b/http.c
index b9666a8..6726b72 100644
--- a/http.c
+++ b/http.c
@@ -2059,6 +2059,41 @@ evhttp_send_error(struct evhttp_request *req, int error, const char *reason)
 #undef ERR_FORMAT
 }
 
+void
+evhttp_send_reply_html(struct evhttp_request *req, int code, const char *fmt, ...)
+{
+	static const char html_header[] =
+		"<HTML><HEAD>\n"
+		"<TITLE>%d %s<TITLE>\n"
+		"</HEAD><BODY>\n"
+		"<H1>%d %s</H1>\n";
+
+	static const char html_footer[] =
+		"</BODY></HTML>";
+
+	va_list ap;
+	const char *phrase = evhttp_response_phrase_internal(code); 
+
+	struct evbuffer *buf = evbuffer_new();
+	if (buf == NULL) {
+		/* if we cannot allocate memory; we just drop the connection */
+		evhttp_connection_free(req->evcon);
+		return;
+	}
+
+	evhttp_response_code(req, code, NULL);
+
+	evbuffer_add_printf(buf, html_header, code, phrase, code, phrase);
+	va_start(ap, fmt);
+	evbuffer_add_vprintf(buf, fmt, ap);
+	va_end(ap);
+	evbuffer_add(buf, html_footer, sizeof(html_footer) - 1);
+
+	evhttp_send_page(req, buf);
+
+	evbuffer_free(buf);
+}
+
 /* Requires that headers and response code are already set up */
 
 static inline void
@@ -2548,38 +2583,16 @@ evhttp_handle_request(struct evhttp_request *req, void *arg)
 		(*http->gencb)(req, http->gencbarg);
 		return;
 	} else {
-		/* We need to send a 404 here */
-#define ERR_FORMAT "<html><head>" \
-		    "<title>404 Not Found</title>" \
-		    "</head><body>" \
-		    "<h1>Not Found</h1>" \
-		    "<p>The requested URL %s was not found on this server.</p>"\
-		    "</body></html>\n"
-
 		char *escaped_html;
-		struct evbuffer *buf;
 
 		if ((escaped_html = evhttp_htmlescape(req->uri)) == NULL) {
 			evhttp_connection_free(req->evcon);
 			return;
 		}
 
-		if ((buf = evbuffer_new()) == NULL) {
-			mm_free(escaped_html);
-			evhttp_connection_free(req->evcon);
-			return;
-		}
-
-		evhttp_response_code(req, HTTP_NOTFOUND, "Not Found");
-
-		evbuffer_add_printf(buf, ERR_FORMAT, escaped_html);
+		evhttp_send_reply_html(req, HTTP_NOTFOUND, "The requested URL %s was not found on this server.", escaped_html); 
 
 		mm_free(escaped_html);
-
-		evhttp_send_page(req, buf);
-
-		evbuffer_free(buf);
-#undef ERR_FORMAT
 	}
 }
 
diff --git a/include/event2/http.h b/include/event2/http.h
index b4176bf..dad5589 100644
--- a/include/event2/http.h
+++ b/include/event2/http.h
@@ -255,6 +255,22 @@ void evhttp_send_error(struct evhttp_request *req, int error,
     const char *reason);
 
 /**
+ * Send an formatted string as a HTML reply to the client.
+ *
+ * The body of the reply will be a preformatted HTML page with
+ * a title containing the passed HTTP status code and it's 
+ * associated RFC phrase, followed by a paragraph containing
+ * a formatted string.
+ *
+ * @param req a request object
+ * @param code the HTTP response status code to send
+ * @param fmt a format string
+ * @param ... arguments that will be passed to printf(3)
+ */
+void evhttp_send_reply_html(struct evhttp_request *req, int code, 
+    const char *fmt, ...);
+
+/**
  * Send an HTML reply to the client.
  *
  * The body of the reply consists of the data in databuf.  After calling