Re: [PATCH] Helper callback params

On 10/26/2012 04:23 AM, Amos Jeffries wrote:
> Updated patch attached for review.

This seems to add a typo bug:

> + } else if (!strncmp(p,"ERR",3) && (p[3] == ' ' || p[3] == '\n' || p[2] == '\0')) {

s/p[2]/p[3]/?

"buf" is guaranteed to be 0-terminated, right?

Should we be worried about \r before \n? Perhaps on Windows?

>>> + // check we have something to parse
>>> + if (!buf || len < 1)
>>> + return;
>> ...
>>> + if (len >= 2) {
>> Would not all the code work if len is zero or one? If yes, remove the
>> len checks, especially if len is going to be more than 1 in 99.9999% of
>> cases.
>
> URL-rewriter which are unfortunatley common still return empty response
> as their most common case.
> The cases where >= 2 will be closer to 50% of overall helper responses
> IMO. Unti that is gone this is an optimization to prevent multiple
> strncmp() being run on 0-1 length strings.

Understood. Please add a comment explaining this. For example,

// optimization: large portion of [URL-rewriter] responses are a single
// character (that we will stuff into other_).
if (len >= 2) ...

However, it feels wrong that we do not check what that character is (or
does the URL rewriting code do that?).

Cheers,

Alex.
Received on Fri Oct 26 2012 - 16:33:53 MDT