The current template...
<HTML><HEAD><TITLE>%s</TITLE>
</HEAD><BODY>
<H1>Method Not Implemented</H1>
Invalid method in request<P>
</BODY></HTML>
is highly confusing. The given title is easily overlooked and the hard-coded content is just plain wrong in most cases (I really read this as "the server did not understand the requested HTTP method).
At the very least I'd prefer something along the lines of:
<HTML><HEAD><TITLE>%d %s</TITLE>
</HEAD><BODY>
<H1>%d An Error Occurred</H1>
%s<P>
</BODY></HTML>
evbuffer_add_printf(buf, ERR_FORMAT, error, reason, error, reason);
... so you at least see what went wrong in the page body. Still I don't think putting the reason after the error code in the title is a good idea as it suggests it's the actual name of the status code.
But best would probably a lookup table for the defined HTTP status code names.
Perfect would be if:
evhttp_send_error(req, 500, "I don't feel like servicing requests right now")
yielded
<HTML><HEAD><TITLE>500 Internal Server Error</TITLE>
</HEAD><BODY>
<H1>500 Internal Server Error</H1>
I don't feel like servicing requests right now<P>
</BODY></HTML>
... although you maybe don't want to include a list of all status code names in libevent. In that case one could another parameter (for the "Internal Server Error" in this case) to evhttp_send_error(), which would unfortunately not compatible...
Sounds reasonable to me. Want to write a patch? I like either of the last two approaches just fine.
Example implementation
(I would have sent this to the mailing list but it appears to be down right now)
I've attached a sample implementation, tell me if you would have any objections to a submission like that.
However, looking at the result some issues come up:
For once, why is evhttp_send_error() named that way? The only difference to evhttp_send_reply is that you pass along a string instead of an evbuffer and it constructs a simple html page around the string.
(There is some redundant code by the way - evhttp_send_error() sets Connection: Close, forgets Content-type, but it all later gets cleared and reset in evhttp_send_page)
You can send simple 200 OK messages with it just fine.
So maybe replace evhttp_send_error() altogether by something like evhttp_send_reply_html()?
Also the other send functions now still have that reason parameter. Should they all have it removed?
I suppose that depends on whether or not it makes sense to send custom response phrases - which I think it doesn't. They are really only informational and ought to be ignored by the other side.
The only use-case I can think of is someone extending HTTP wishing to provide response phrases for his custom codes - but I think in this case it makes more sense to provide a callback which is called by my new evhttp_response_phrase_internal() in case the response message for an unknown code is requested.
So, what I would do - ignoring compatibility here:
Remove the reason parameters at the send functions altogether, let evhttp_response_code() figure it out from evhttp_response_phrase_internal(). evhttp_response_code() has access to the request through which it can later obtain a callback for custom phrases, if that feature is indeed wanted.
As a bonus you lose the dynamic memory management for response_code_line.
Replace evhttp_send_error() by evhttp_send_reply_html(). Remove the dead header code.
And when all that is done... implement evbuffer_add_vprintf(), make evhttp_send_reply_html() use it and take varargs, and then remove the duplicated code from evhttp_handle_request() which generates the 404 reply.
I apologize for planning to take your code apart. :-)
So, any objections? (I fear you might have compatibility issues with this...)
If compatibility bothers you:
For max compatibility the send functions could still take the parameter, and the phrase auto-resolve only kicks in when NULL is passed. Unfortunately you then either have to keep the dynamic memory management for response_code_line or add additional free logic...
So... what do you think?
For the mailing list, you tried libevent-users@freehaven.net, right? (Subscription instructions are at http://archives.seul.org/libevent/users/ .) The @monkey.org list has been down for some time.
Renaming the function or changing its interface in a backward-incompatible is NOT an option; we try very hard not to break existing code. (Imagine how annoyed you would be if you wrote a perfectly good program with Libevent, and then it broke next month because somebody convinced us that one of the functions you wanted to use had a badly chosen name.)
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.
(Other than that, what do you mean by "implement evbuffer_add_vprintf"? It already exists.)
Further discussion in http://archives.seul.org/libevent/users/May-2010/msg00020.html and in its replies. I've applied Felix's first revised patch, and am uploading the second one here so that we don't lose track of it between now and 2.1.x