Hi Jarno,
On Mon, Mar 06, 2017 at 04:15:00PM +0200, Jarno Huuskonen wrote:
> Hi Willy,
>
> On Fri, Feb 10, Willy Tarreau wrote:
> > > How should I send the patches ? One commit for
> > > http_server_error/http_get_status_idx changes and tarpit deny_status
> > > parser / doc in another commit ?
> >
> > Yes that's the prefered way to do it, one commit per architecture or
> > functional change to ease review and bug tracking later.
>
> I'm including two commits for this feature.
> - 0001-MEDIUM-http_error_message-txn-status-http_get_status.patch
> Removes second argument from http_error_message and uses
> txn->status / http_get_status_idx to map the 200..504 status code
> to HTTP_ERR_200..504 enum.
> (http_get_status_idx has default return value of HTTP_ERR_500, is
> this ok ?)
Yep that sounds OK. Anyway any unhandled error is an haproxy bug so it
indeed should return a 500 :-)
>
> - 0002-MINOR-http-request-tarpit-deny_status.patch
> Adds http-request tarpit deny_status functionality.
> (depends on the 0001-... commit).
Thank you, this all looks good, I've now merged them. I'm even seeing an
opportunity for slightly simplifying things further : http_server_error()
sets the status in txn->status from what it received in argument, but in
practice you had to prepare txn->status before calling it due to
http_error_message(). So I'll now get rid of this assignment and make
http_server_error() retrieve the status from the txn instead, that will
be more consistent. There's a single call place where it was not yet done,
its the server redirect.
> Alternative implementation (0001-....) for http_return_srv_error
> could be something like this:
> void http_return_srv_error(struct stream *s, struct stream_interface *si)
> {
> int err_type = si->err_type;
> int send_msg = 1;
>
> if (err_type & SI_ET_QUEUE_ABRT)
> http_server_error(s, si, SF_ERR_CLICL, SF_FINST_Q, 503, NULL);
> else if (err_type & SI_ET_CONN_ABRT) {
> if (s->txn->flags & TX_NOT_FIRST)
> send_msg = 0;
> http_server_error(s, si, SF_ERR_CLICL, SF_FINST_C, 503, NULL);
> }
> ...
> ...
> else /* SI_ET_CONN_OTHER and others */
> http_server_error(s, si, SF_ERR_INTERNAL, SF_FINST_C, 500, NULL);
>
> if (send_msg)
> bo_putchk(s->res.buf, http_error_message(s));
> }
I'm not at all convinced that it provides any benefit over your current
implementation, so let's stick with what you already have.
Thanks!
Willy
