Re: [PATCH] Helper callback params

Updated patch attached for review.

On 26/10/2012 6:53 p.m., Alex Rousskov wrote:
> On 10/25/2012 06:32 PM, Amos Jeffries wrote:
>> Version 5, this includes most of the HelperReply changes brought up
>> earlier.
> Hi Amos,
>
> I think there may be one or two bugs here. The rest are just a few
> simplifications or polishing touches not requiring another review round IMO:
>
>
>> + // 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.

>
>> + // NOTE: only increment 'p' if a result code is found.
> That seems obvious. It would be even more obvious if you renamed p to
> something more meaningful like "blob" or "blobStart".

This is just a local iterator variable. It wont always be a blob or
such. When we are dealing with a blob (end of parse function) the
variable is named for its content.
Common practice I've seen is to use p for iterator over a char*.

>
>> + if (!strncmp(p,"OK ",3)) {
>> + result = HelperReply::Okay;
>> + p+=2;
>> + } else if (!strncmp(p,"ERR ",4)) {
>> + result = HelperReply::Error;
>> + p+=3;
> ...
>> + for(;xisspace(*p);p++); // skip whitespace
> You already know the space character is there after these strncmp()
> calls so you should increment p by the full amount (3 and 4 in the above
> two cases) instead of checking the same space character again in the
> xisspace() loop later. However, see below for a concern about that space
> being required.

True. Delimiter is SP, CR, or NUL.

Have fixed for TT, AF, NA where only SP delimiter is relevant. The
others are left as explained below...

>
>> The response format acceptible from any helper is now:
>> [channel-ID] [result] [blob] <terminator>
>> + if (!strncmp(p,"OK ",3)) {
> So the helper cannot just send, for example, "OK\n", without a trailing
> space or anything else on the response line? The syntax you mentioned in
> the patch description (also quoted above) says that it can.

Er. Yes some of them could.
Added specific byte checks for SP and the helper EOL which are valid
delimiters there after each

This also speaks to why p+= did not increment it over the delimiter. So
that CR NUL are still visible to later code after the response code
handling.

>
>> + for(;xisspace(*p);p++); // skip whitespace
> Use ++prefix increment so that we do not do another round of increment
> changes :-).

Fixed.

>> + const mb_size_t blobSize = (buf+len-p);
>> + other_.init(blobSize, blobSize+1);
> If blobSize is zero (which can happen at least on malformed responses),
> the above init() call will assert. See below for a possible fix with a
> nice side effect.
>
>
>> + other_.append(p, blobSize); // remainders of the line.
>> +
>> + // NULL-terminate so the helper callback handlers do not buffer-overrun
>> + other_.terminate();
> If we always terminate, then we always need that extra 1-byte space in
> there. Consider calling init(blobSize+1, blobSize+1).

I don't intend to always have it terminate. But yes, done that way in
the meanwhile.

>
>> + // NULL-terminate so the helper callback handlers do not buffer-overrun
>> + other_.terminate();
> But we do _not_ terminate if (!buf || len < 1). We do not even
> init()ialize the "other_" buffer in that case. Is that OK? Should we
> always initialize and terminate() to be on the safe side, despite the
> overheads? I see quite a few HelperReply(NULL,0) calls so those objects
> with uninitialized other_ blobs will exist.
>
>
>> + ~HelperReply() {}
> Please remove. The compiler will provide [a more efficient version of] it.

Okay.

>
>> - assert(lm_request->authserver == lastserver);
>> + assert(lm_request->authserver == reply.whichServer.raw());
> If you swap "==" operands, can you avoid the .raw() call?
> assert(reply.whichServer == lm_request->authserver);

It appears that we can. Done.

Amos