[squid-dev] [PATCH] Fix ext_session_acl to handle - when no argument is passed

[Amos Jeffries]

On 23/03/2017 7:10 a.m., Alex Rousskov wrote:
> On 03/22/2017 06:44 AM, Amos Jeffries wrote:
> 
>> This is a side effect of the change to using logformat tokens. The
>> %DATA field is always present now.
> 
> My understanding is that, after the logformat changes, Squid started
> appending "-" to the helper query when an external ACL has no
> parameters. The examples use parameterless ACLs to indicate that no or
> default action is to be taken (depending on active/passive mode). We now
> need to adjust the helper code to _expect_ that dash.

Yes, that matches my understanding of the situation.

> 
>> +            } else if (!default_action && strcmp(lastdetail, " -") == 0) {
>> +                // no action; LOGIN/LOGOUT not supplied
>> +                // but truncate the '-' %DATA value given by Squid-4 and later
>> +                detail_len = (size_t)(lastdetail-detail);
>> +                *lastdetail = '\0';
>>              }
>> +            // else do nothing
> 
> s/truncate/remove/?
> 

Okay. Will do.

> It is not clear why we need to remove the dash only when default_action
> is zero (i.e., in active mode). The dash is going to be present in both
> modes, right? I am not saying the patch is wrong; only that it is not
> clear why the code does what it does.

This helpers' passive mode does not need to care what the input is, so
long as it is consistent. The !default_action makes it avoid the extra
work if we dont need to care about it.

The admin may still use %DATA in either mode, so not touching the input
seems to be the better way to go except where there is no choice.

> 
> I would expect something along these lines instead (pseudo code):
> 
>   // What action was requested (including no/default action)?
>   if (... LOGIN ...) action = 1;
>   else if (... LOGOUT ...) action = -1;
>   else if (... - ...) action = default_action;
>   else { ... BH unexpected query format ...; continue; }
> 

That will result in BH for passive mode when the admin wants to use
%DATA in the format string (unless it is last).

The "// else do nothing" is explicitly added by my patch to document
that the case of unknown other values is valid/expected and to not touch
the contents of 'detail' when it happens.


Amos