Re: [PATCH] Helper callback params

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.
Received on Fri Oct 26 2012 - 05:53:49 MDT