Re: external_acl.cc

[Henrik Nordstrom]

On Mon, 23 Jan 2006, Gonzalo Arana wrote:

Hi,
I'm sory to bother you, but I rather ask you directly instead of
asking this to the list:

As I inidcated previously I prefer public flaming when I am in error ;-)

while trying to hunt bug# 1491, I've noticed this in external_acl.cc
(function externalAclHandleReply()):
...
if (reply)
 entry = external_acl_cache_add(state->def, state->key, entryData);
else {
   if (reply)
**    entry = (external_acl_entry *)hash_lookup(state->def->cache, state->key);
   else {
     external_acl_entry *oldentry =
       (external_acl_entry *)hash_lookup(state->def->cache, state->key);

Unless I am reading this bad, the line marked with ** is never
executed, am I right?

Right..

This was added by you (unless I am mistaken) while fixing bug# 577, on
revision 1.35 of external_acl.cc, see:
http://www.squid-cache.org/cgi-bin/cvsweb.cgi/squid3/src/external_acl.cc.diff?r1=1.35&r2=1.36

I guess that the second 'if (reply)' should be deleted, am I right?

The whole if block should be unwinded it seems. Looks like a bad port of a 
Squid-2.5 patch.

The corresponding 2.5 code reads:

    if (cbdataValid(state->def)) {
        if (reply)
            entry = external_acl_cache_add(state->def, state->key, result, 
user, error);
        else {
            external_acl_entry *oldentry = hash_lookup(state->def->cache, 
state->key);
            if (oldentry)
                external_acl_cache_delete(state->def, oldentry);
        }
    }

which makes more sense..

Squid-3 has now been corrected to match this.

Regards
Henrik