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.

Okay, we need to take a closer look at it sometime in future then. to
figure out the two APIs properly. Good enough for now.

>
> [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.

I did notice that. But we still need to fix the updated code as we go
around finding things like this.

Amos
Received on Mon Sep 22 2008 - 23:50:08 MDT