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.
> + // 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".
> + 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.
> 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.
> + for(;xisspace(*p);p++); // skip whitespace
Use ++prefix increment so that we do not do another round of increment
changes :-).
> + 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).
> + // 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.
> - 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);
Thank you,
Alex.
