Re: [PATCH] Bug 1488: squid_status ACL

On 11/05/2011 04:50 PM, Amos Jeffries wrote:
> On 5/08/2011 2:36 a.m., Amos Jeffries wrote:
>> This updates and completes the work started by Gonzalo Arana in
>> creating an ACL to match the TCP_HIT, TCP_MISS, etc internal status of
>> request handling.
>>
>> Requests can change their status over the course of a transaction.
>> Particularly around retries and revalidations. This ACL seeks to match
>> the current state as of the point of testing. The most reliable place
>> is of course in the logging ACL tests after any state changes have
>> stopped. Although any point after parsing may match on a relevant status.
>>
>> To support this the squid status enum and supporting functionality is
>> also moved into its own file.
>>
>>
>> NOTES:
>> At this point I'm not sure the main code is updating the status
>> correctly at all points it should change. Deferred that large audit task.
>>
>> There is also some questions about the states currently described vs
>> the ones that exist in Squid. ie should the enum be extended to a
>> larger set with the full state permutations, or split into a bitmap
>> that can support match on a subgroups of states (ie all HIT, all REFRESH)
>>
>>
>> Amos
>
> If there are no objections I'll freshen and commit this patch later today.
>
> Amos

> +// TODO: this should be a bitmap for better optimization
> +typedef enum {

Missing description for the type. I also do not like the SquidStatus
name (which tells pretty much nothing given that we are always inside
Squid and there are dozens of possible "status"es). I appreciate that it
is difficult to define/describe/name exactly, but even "logType" was
better than "SquidStatus"!

> + SquidStatus logType;

The new member should have a description.

> +inline SquidStatus
> +operator ++(SquidStatus &i)
> +{
> + return (SquidStatus)(i+1);
> +}
> +

The ++ operator should increment its argument. The fact that it does
not, probably implies that the patch was poorly tested -- it should not
have worked as all iteration loops would have run forever.

> +ACLSquidStatusData::ACLSquidStatusData(ACLSquidStatusData const &old)
> +{
> + memcpy(values, old.values, sizeof(values) );
> +}
> +
> +ACLSquidStatusData::~ACLSquidStatusData()
> +{ }
> +

The two methods above are not needed.

> + ACLSquidStatusData &operator= (ACLSquidStatusData const &);

The above is declared but not implemented. It is also not needed.

> + char *t = NULL;
> +
> + while ((t = strtokFile())) {

The t variable can be declared inside the while() condition.

> #endif
> + http->getConn()->flags.readMore = true; // resume any pipeline reads.
> node = (clientStreamNode *)http->client_stream.tail->data;
> clientStreamRead(node, http, node->readBuffer);
> return;
> @@ -1314,7 +1315,7 @@
> {
> PROF_start(httpStart);
> logType = LOG_TAG_NONE;
> - debugs(85, 4, "ClientHttpRequest::httpStart: " << Format::log_tags[logType] << " for '" << uri << "'");
> + debugs(85, 4, HERE << logType << " for '" << uri << "'");
>
> /* no one should have touched this */
> assert(out.offset == 0);
> @@ -1773,7 +1774,7 @@
> #endif
>
> request->detailError(ERR_ICAP_FAILURE, errDetail);
> -
> + c->flags.reaMore = true;
> node = (clientStreamNode *)client_stream.tail->data;
> clientStreamRead(node, this, node->readBuffer);
> }

Out of scope?

HTH,

Alex.
Received on Sun Nov 06 2011 - 18:48:14 MST