Re: [PATCH] Helper callback params

Updated patch attached for audit with changes as discussed below.

On 27/10/2012 5:33 a.m., Alex Rousskov wrote:
> 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]/?

Oops. Removed as below...

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

In a way yes. The EOL delimiter (whatever it is) gets replaced with \0
in helperHandleRead() before being passed to the HelperReply parser. But
there are helpers like ssl_crtd or pinger which may send back tokens
containing \0 so it is not dependable null-termination so much as a
safety backup for str*() functions being used. The "len" parameter tells
us far better where the end of line actually is...

Which brings to mind a far better solution. Thank you for making me
re-think this so much.
Since the tokens we are looking for are followed by SP or the only thing
on the line we can do this without worrying what EOL might be:

"
         if (!strncmp(p,"OK",2) && (len == 2 || p[2] == ' ')) {
             result = HelperReply::Okay;
             p+=2;
         } else if (!strncmp(p,"ERR",3) && (len == 3 || p[3] == ' ')) {
             result = HelperReply::Error;
             p+=3;
         } else if (!strncmp(p,"BH",2) && (len == 2 || p[2] == ' ')) {
"

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

The whole \r\n is treated as \n and replaced by helperHandleRead()
before getting here. In all other cases \r might be part of some blob we
should not be matching.

>
>>>> + // 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?).

It does no specific checks AFAICT. But the case needs to be written
through anyway to retain the old behaviour on those responses - even if
it was breaking.

Amos