Hi Jarno, On Mon, Feb 06, 2017 at 04:33:01PM +0200, Jarno Huuskonen wrote: > Hi, > > On Mon, Jan 16, Willy Tarreau wrote: > > > The second patch updates http_process_req_common/http_process_tarpit to > > > use deny_status. http_process_tarpit has a switch statement for mapping > > > txn->status (200<->504) to HTTP_ERR_<num>(enum). Is this reasonable ? > > > > Yes it's reasonable however it would be a good idea to instead write a > > short function in charge of returning this index based on the status code > > and then to use it everywhere we need this index, including in your own > > code. When you look at http_error_message(), everywhere it's called, > > txn->status already contains the status code, except in one place : > > http_return_srv_error() where we set the status code. Thus I'd proceed > > like this : > > > > 1) change http_server_error() to set the status code separately from the > > message : > > > > if (status > 0) > > s->txn->status = status; > > if (msg) > > bo_inject(si_ic(si), msg->str, msg->len); > > > > 2) implement your switch/case as a function (eg: http_get_status_idx()) > > > > 3) make http_error_message() pass s->txn->status to htpt_get_status_idx() > > to retrieve the message index, and drop its second argument ; modify > > http_return_srv_error() to pass NULL to all calls to http_server_error() > > instead of http_error_message(), and add a final call in this function > > to do this : > > > > bo_putchk(&s->res, http_error_message(s)); > > Do you mean that bo_putchk should go to end of http_return_srv_error ?
I don't remember, I have to reread the code :-/ > I don't think &s->res is correct ? It produces a compiler warning: > struct buffer * vs struct channel * > and coredumps if backend server is down and haproxy should return 503. > > What seems to work (very minimal testing) is: > bo_putchk(s->res.buf, http_error_message(s)); Ah you're right apparently this one takes a buffer. > Does the attached patch look otherwise ok ? I think there's a problem in http_error_message(), there were some conditions to detect certain error cases on reused connections for which we had to remain silent and these conditions have disappeared so we'll return an error instead of silently closing. > (Do I need to add http_get_status_idx prototype to > include/proto/proto_http.h ?) It depends if it's used somewhere else or if its scope really makes no sense out of this file. A good rule of thumb is to say that if your function is not used outside of the C file, mark it static. That way we don't even expect to find it outside. If someone later needs it, they'll simply have to remove the static and export the function but at least there will be no confusion around it. > 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. Thanks! Willy
