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

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?

> +typedef enum {
> + HTCP_CLR_PURGE,
> + HTCP_CLR_INVALIDATION,
> +} htcp_clr_reason;
> +#endif
> +
> #endif /* SQUID_ENUMS_H */
>
> === modified file 'src/htcp.cc'
> --- src/htcp.cc 2008-09-08 23:52:06 +0000
> +++ src/htcp.cc 2008-09-22 00:53:13 +0000
> @@ -176,6 +176,7 @@
> int rr;
> int f1;
> int response;
> + int reason;
> u_int32_t msg_id;
> htcpSpecifier S;
> htcpDetail D;
> @@ -248,8 +249,6 @@
>
> static void htcpHandle(char *buf, int sz, IPAddress &from);
>
> -static void htcpHandleData(char *buf, int sz, IPAddress &from);
> -
> static void htcpHandleMon(htcpDataHeader *, char *buf, int sz, IPAddress &from);
>
> static void htcpHandleNop(htcpDataHeader *, char *buf, int sz, IPAddress &from);
> @@ -439,6 +438,26 @@
> }
>
> static ssize_t
> +htcpBuildClrOpData(char *buf, size_t buflen, htcpStuff * stuff)
> +{
> + u_short reason;
> +
> + switch (stuff->rr) {
> + case RR_REQUEST:
> + debugs(31, 3, "htcpBuildClrOpData: RR_REQUEST");
> + reason = htons((u_short)stuff->reason);
> + xmemcpy(buf, &reason, 2);
> + return htcpBuildSpecifier(buf + 2, buflen - 2, stuff) + 2;
> + case RR_RESPONSE:
> + break;
> + default:
> + fatal_dump("htcpBuildClrOpData: bad RR value");
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t
> htcpBuildOpData(char *buf, size_t buflen, htcpStuff * stuff)
> {
> ssize_t off = 0;
> @@ -451,7 +470,7 @@
> break;
>
> case HTCP_CLR:
> - /* nothing to be done */
> + off = htcpBuildClrOpData(buf + off, buflen, stuff);
> break;
>
> default:
> @@ -1287,13 +1306,82 @@
> htcpFreeSpecifier(s);
> }
>
> +/*
> + * Forward a CLR request to all peers who have requested that CLRs be
> + * forwarded to them.
> + */
> static void
> +htcpForwardClr(char *buf, int sz)
> +{
> + peer *p;
> +
> + for (p = Config.peers; p; p = p->next) {
> + if (!p->options.htcp) {
> + continue;
> + }
> + if (!p->options.htcp_forward_clr) {
> + continue;
> + }
> +
> + htcpSend(buf, sz, p->in_addr);
> + }
> +}
>
> -htcpHandleData(char *buf, int sz, IPAddress &from)
> +/*
> + * 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)

> {
> + 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?

> +
> + return;
> + }
> +
> + if (htcpHdr.major != 0)
> + {
> + debugs(31, 1, "htcpHandle: Unknown major version " << htcpHdr.major << " from " << from );
> +

Same here.

> + return;
> + }
> +
> + hbuf = buf + sizeof(htcpHeader);
> + hsz = sz - sizeof(htcpHeader);
> +
> + if ((size_t)hsz < sizeof(htcpDataHeader))
> {
> debugs(31, 1, "htcpHandleData: msg size less than htcpDataHeader size");

and same here again.

> return;
> @@ -1301,11 +1389,10 @@
>
> if (!old_squid_format)
> {
> - xmemcpy(&hdr, buf, sizeof(hdr));
> - } else
> - {
> + xmemcpy(&hdr, hbuf, sizeof(hdr));
> + } else {
> htcpDataHeaderSquid hdrSquid;
> - xmemcpy(&hdrSquid, buf, sizeof(hdrSquid));
> + xmemcpy(&hdrSquid, hbuf, sizeof(hdrSquid));
> hdr.length = hdrSquid.length;
> hdr.opcode = hdrSquid.opcode;
> hdr.response = hdrSquid.response;
> @@ -1317,11 +1404,10 @@
>
> hdr.length = ntohs(hdr.length);
> hdr.msg_id = ntohl(hdr.msg_id);
> - debugs(31, 3, "htcpHandleData: sz = " << sz);
> + debugs(31, 3, "htcpHandleData: hsz = " << hsz);
> debugs(31, 3, "htcpHandleData: length = " << hdr.length);
>
> - if (hdr.opcode >= HTCP_END)
> - {
> + if (hdr.opcode >= HTCP_END) {
> debugs(31, 1, "htcpHandleData: client " << from << ", opcode " << hdr.opcode << " out of range");

same here.

> return;
> }
> @@ -1332,8 +1418,7 @@
> debugs(31, 3, "htcpHandleData: RR = " << hdr.RR);
> debugs(31, 3, "htcpHandleData: msg_id = " << hdr.msg_id);
>
> - if (sz < hdr.length)
> - {
> + if (hsz < hdr.length) {
> debugs(31, 1, "htcpHandleData: sz < hdr.length");

same here. I suspect most of these are similar, yes?

If they are not really that important, bump them to something >2.

<snip>
> mb.clean();
> -
> if (!pktlen) {
> debugs(31, 1, "htcpQuery: htcpBuildPacket() failed");

same here.

> return;
> }
> -
> +
> htcpSend(pkt, (int) pktlen, p->in_addr);
>
> queried_id[stuff.msg_id % N_QUERIED_KEYS] = stuff.msg_id;
> @@ -1580,6 +1590,77 @@
> }
>
> /*
> + * Send an HTCP CLR message for a specified item to a given peer.
> + */
> +void
> +htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestMethod &method, peer * p, htcp_clr_reason reason)
> +{
> + static char pkt[8192];
> + ssize_t pktlen;
> + char vbuf[32];
> + htcpStuff stuff;
> + HttpHeader hdr(hoRequest);
> + Packer pa;
> + MemBuf mb;
> + http_state_flags flags;
> +
> + if (htcpInSocket < 0)
> + return;
> +
> + old_squid_format = p->options.htcp_oldsquid;
> + memset(&flags, '\0', sizeof(flags));
> + snprintf(vbuf, sizeof(vbuf), "%d/%d",
> + req->http_ver.major, req->http_ver.minor);
> + stuff.op = HTCP_CLR;
> + stuff.rr = RR_REQUEST;
> + stuff.f1 = 0;
> + stuff.response = 0;
> + stuff.msg_id = ++msg_id_counter;
> + switch (reason) {
> + case HTCP_CLR_INVALIDATION:
> + stuff.reason = 1;
> + break;
> + default:
> + stuff.reason = 0;
> + break;
> + }
> + stuff.S.method = (char *) RequestMethodStr(req->method);
> + if (e == NULL || e->mem_obj == NULL) {
> + if (uri == NULL) {
> + return;
> + }
> + stuff.S.uri = xstrdup(uri);
> + } else {
> + stuff.S.uri = (char *) e->url();
> + }
> + stuff.S.version = vbuf;
> + if (reason != HTCP_CLR_INVALIDATION) {
> + HttpStateData::httpBuildRequestHeader(req, req, e, &hdr, flags);
> + mb.init();
> + packerToMemInit(&pa, &mb);
> + hdr.packInto(&pa);
> + hdr.clean();
> + packerClean(&pa);
> + stuff.S.req_hdrs = mb.buf;
> + } else {
> + stuff.S.req_hdrs = NULL;
> + }
> + pktlen = htcpBuildPacket(pkt, sizeof(pkt), &stuff);
> + if (reason != HTCP_CLR_INVALIDATION) {
> + mb.clean();
> + }
> + if (e == NULL) {
> + xfree(stuff.S.uri);
> + }
> + if (!pktlen) {
> + debugs(31, 1, "htcpClear: htcpBuildPacket() failed");

same here.

> + return;
> + }
> +
> + htcpSend(pkt, (int) pktlen, p->in_addr);
> +}
> +
> +/*
> * htcpSocketShutdown only closes the 'in' socket if it is
> * different than the 'out' socket.
> */
>

Amos

-- 
Please use Squid 2.7.STABLE4 or 3.0.STABLE9