Facility to report detailed parsing errors.

[pgriess]

• Add http_errno enum w/ values for many parsing error conditions.
• Add http_parser_result struct which tracks errno value and line number
  where error occurred; add http_parser_execute_result() which takes
  this as an outparam. Use extra struct vs. augmenting http_parser to
  keep per-conn state small.
• Add http_errno_*() methods to help turning errno values into
  human-readable messages.

[pgriess]

I'm not in love with the API changes, but I think the functionality is right.

I'd have preferred to dump the result data into http_parser but didn't want to inflate the struct since I know you worked hard to make it so small. That said, it's just 8 extra bytes ;)

Also, I opted against creating RETURN_SET_ERRNO() and GOTO_SET_ERRNO() macros as I wanted to keep flow-control logic visible in the main function body.

[mnot]

+1 from an HTTP angle.

[pgriess]

Any thoughts on this? Happy to iterate on it.

[clifffrey]

I don't really like adding the extra argument to http_parser_execute. My gut feeling says to do one of three things instead:

1. Just add a uint8_t field to the http_parser struct that contains the errno. I would drop the line number I think.
2. Use the "state" field. Just have the error numbers go from 128 and up or something. You could add a function http_parser_get_errno that would return 0 if state is < 128, and return state - 128 otherwise.

The downside of (2) is that it makes the error non-recoverable, but I am having trouble imagining a use-case for recoverable errors.

I think that this is awesome data to add, that will be very useful for logging of errors. I don't really like the line number because that is not what we want to maintain/reveal.

[mnot]

There are a couple of recoverable HTTP parsing errors; e.g., the header line without a colon (that's not a continuation), as discussed recently on-list.

I agree, however, that they're pretty rare, and in the 99% case you can determine a single behaviour that's good for everyone. The exception is if someone wants to build a validator or HTTP traffic inspector using the parser; in that case you really need that deep information, and you need to be able to recover (so that one error doesn't stop inspection).

However, this is a very specialised use case (obviously) that it'd be easy to say is just out of scope.

[ry]

I like @clifffrey's (2) suggestion

[pgriess]

Ok, I'll go with (2). I hate the API mangling that went into the existing change anyway ;)

I do, however, feel somewhat strongly about exposing source-level information. This is quite useful for debugging (e.g. to figure out which of the 12 callsites that set INVALID_VERSION w/o setting a breakpoint on all of them). I'll update the pull with a a uint16_t line number that's visible only under (the to be created) HTTP_PARSER_DEBUG flag and see what you guys think.

[clifffrey]

The change looks good to me.
It would be cool to change test_simple to specifically take the error code that we expect to be returned, but the change is worth checking in even without that.