Re: [MERGE] Address comments from Alex and Amos.

On 22/09/2008, at 11:26 PM, Amos Jeffries wrote:

> Benno Rice wrote:
>> Address comments from Alex and Amos.
>> - Add some comments describing various function purposes.
>> - Remove some debugging debugs that had crept in.
>> - Use debugs() in preference to debug()().
>> - Adjust some debug levels.
>
> Getting close.
>
> <snip>
>> === modified file 'src/enums.h'
>> --- src/enums.h 2008-07-11 20:43:43 +0000
>> +++ src/enums.h 2008-09-01 05:27:59 +0000
>> @@ -545,4 +545,15 @@
>> DISABLE_PMTU_TRANSPARENT
>> };
>> +#if USE_HTCP
>> +/*
>> + * This should be in htcp.h but because neighborsHtcpClear is
>> defined in
>> + * protos.h it has to be here.
>> + */
>
> So why is neighborsHtcpClear outside htcp.h then?
>>

Because it's not a piece of HTCP functionality. It lives in
neighbors.cc with the rest of the general peer-related code. The
equivalent ICP functions live there too, IIRC. Do you think it should
move? It should probably be renamed if so.

[snip]

>> +/*
>> + * Do the first pass of handling an HTCP message. This used to be
>> two
>> + * separate functions, htcpHandle and htcpHandleData. They were
>> merged to
>> + * allow for forwarding HTCP packets easily to other peers if
>> desired.
>> + *
>> + * This function now works out what type of message we have
>> received and then
>> + * hands it off to other functions to break apart message-specific
>> data.
>> + */
>> +static void
>> +htcpHandle(char *buf, int sz, IPAddress &from)
>
> Um, sorry for not picking this up earlier. With that documentation,
> I think the function should probably be called htcpHandleMsg or
> similar to describe what its handling.
> (ie, FTP use HandleControlReply)

I just kept the original name of the function. I'll change it as you
describe however.

>> {
>> + htcpHeader htcpHdr;
>> htcpDataHeader hdr;
>> -
>> - if ((size_t)sz < sizeof(htcpDataHeader))
>> + char *hbuf;
>> + int hsz;
>> + assert (sz >= 0);
>> +
>> + if ((size_t)sz < sizeof(htcpHeader))
>> + {
>> + debugs(31, 1, "htcpHandle: msg size less than htcpHeader
>> size");
>> + return;
>> + }
>> +
>> + htcpHexdump("htcpHandle", buf, sz);
>> + xmemcpy(&htcpHdr, buf, sizeof(htcpHeader));
>> + htcpHdr.length = ntohs(htcpHdr.length);
>> +
>> + if (htcpHdr.minor == 0)
>> + old_squid_format = 1;
>> + else
>> + old_squid_format = 0;
>> +
>> + debugs(31, 3, "htcpHandle: htcpHdr.length = " <<
>> htcpHdr.length);
>> + debugs(31, 3, "htcpHandle: htcpHdr.major = " << htcpHdr.major);
>> + debugs(31, 3, "htcpHandle: htcpHdr.minor = " << htcpHdr.minor);
>> +
>> + if (sz != htcpHdr.length)
>> + {
>> + debugs(31, 1, "htcpHandle: sz/" << sz << " !=
>> htcpHdr.length/" <<
>> + htcpHdr.length << " from " << from );
>
> Level 1 is admin visible.
> What does it mean about their Squid's operation that its this
> important? what can they do to fix it?

[snip]

All of these were here previously in exactly this form. I did not add
any of them. I can change them all but if you examine the diff you'll
see that any I appear to have added (such as this one) are actually
due to the code rearrangement.

New merge will be posted shortly.

-- 
Benno Rice
benno_at_jeamland.net