Hi Jarno,
On Wed, Feb 01, 2017 at 07:52:57PM +0200, Jarno Huuskonen wrote:
> Does it make sense to try to optimize the switch statement
> and have most common status codes at the top ? Something like
> switch(txn->status) {
> case 500:
> http_err_code_idx = HTTP_ERR_500;
> break;
> case 403:
> http_err_code_idx = HTTP_ERR_403;
> break;
> ...
No it does not because anyway they are not evaluated sequentially, the
compiler will often sort them in a tree-like method in order to limit
the number of evaluations. You will notice that the code is exactly the
same whatever the order you use.
> > 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));
> >
> > This way adding new status codes will be more straight forward and will
> > not require to touch multiple places all the time.
>
> Sounds good, I'll (try to) work on this over the weekend.
Great!
> > However I'm seeing the config parser become really ugly around the
> > deny_status part (it already was but now it's worse).
> >
> > What would you think about removing the rule->deny_status = HTTP_ERR_403
> > entry that's hidden before the rules are parsed, and to place this and
> > the action this way under the deny/block/tarpit "if" block :
> >
> > + if (!strcmp(args[0], "tarpit")) {
> > + rule->action = ACT_HTTP_REQ_TARPIT;
> > + rule->deny_status = HTTP_ERR_500;
> > + }
> > + else {
> > + rule->action = ACT_ACTION_DENY;
> > + rule->deny_status = HTTP_ERR_403;
> > + }
> >
> > Also, you can simplify this part :
> >
> > > @@ -9031,13 +9035,10 @@ struct act_rule *parse_http_req_cond(const char
> > > **args, const char *file, int li
> > > }
> > >
> > > if (hc >= HTTP_ERR_SIZE) {
> > > - Warning("parsing [%s:%d] : status code
> > > %d not handled, using default code 403.\n",
> > > - file, linenum, code);
> > > + Warning("parsing [%s:%d] : status code
> > > %d not handled, using default code %d.\n",
> > > + file, linenum, code,
> > > rule->action == ACT_ACTION_DENY ? 403: 500);
> > > }
> >
> > Simply pass "rule->deny_status" instead of the condition, you'll always
> > report the status that is programmed to be returned, it will always be
> > more accurate and makes the code simpler.
>
> I'm including a new patch for the parser changes. Is this what you had
> in mind for the parser ?
Yes that's it. Sorry, I believed that the deny_status contained the status
code itself, but what you did is right.
Thanks,
Willy
