Re: [PATCH v1] unit-tests for request first-line parser.

On Sun, 29 Aug 2010 19:22:02 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 08/29/2010 05:09 PM, Amos Jeffries wrote:
>
>>> * FooInit() is a C version of a Foo class constructor. I would
recommend
>>
>>> converting HttpParserInit() to a constructor since you are polishing
>>> this code anyway, but if you leave init() in place, please call
clear()
>>> _after_ performing any initializations in init(). This approach makes
a
>>> future conversion much easier.
>>>
>>
>> Um, no.
>>
>> init() sets it to a state where data is available for parsing and
about
>> to begin.
>>
>> clear() sets it to a state where there is no data available and
parsing
>> is invalid.
>
> For parsers, it is a bad idea to equate "no data available" with
> "invalid", but, at any rate, the clear() method is misnamed: it is a
> standard name in STL and other libraries and never implies invalidation,

> just emptying and resetting. Your clear, if it must invalidate, should
> be called invalidate() or something. And calling it from init() becomes
> very confusing; please avoid that if clear() is actually invalidate().
>
>
>> HttpParser is (ab)used in the current code by being re-used for
multiple
>> requests and responses *without clearing previous offset memory*.
>
> What do you mean by "offset memory"? Does not init() resets offsets and
> pointers?

By "offset memory" I mean the values from previous runs that HttpParser
stores for offset to various pieces of the request line.

init() reset the overall start/end offsets known for the line, but left
the finer details like start/end offsets in held for method/URL/version at
some random value from a previous run or in the case of a unused
HttpParser, pointing at some random offset in RAM heap space relative to
the about to be processed read buffer.

>
>> It has no
>> buffers of its own, it merely points at the callers buffer space and
hold
>> offsets. Also, that callers buffer may at any time be re-allocated.
>> Current
>> code gets around this by re-init().
>
>> Okay, for the basic fix+tests I'll remove clear() again. Leaving it and
>> your comments to the polish stage.
>> NOTE: we then cannot test for incorrect setting of supposedly unset
>> fields.
>
>
> Alex.
Received on Mon Aug 30 2010 - 03:08:35 MDT