On 03/22/2017 07:53 PM, Amos Jeffries wrote: > This helpers' passive mode does not need to care what the input is, so > long as it is consistent.
I do not recommend ignoring input format in hope that ignored input is consistent/acceptable/compatible/etc. However, I am not going to object to any changes to this helper, even if they violate that general principle, so feel free to commit whatever you think is the best. I do not plan on following this thread further. > 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. Using %DATA explicitly at the expected location (i.e., last) is not a problem. Using it in unexpected and possibly unsupported places is. >> 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). Yes, and I think that would be the right behavior for the helper. The only counter argument that I can think of is that undocumented(?) passive helper functionality correctly uses (not just tolerates/ignores but _uses_) arbitrary query input past the source address. It is difficult for me to imagine a good helper design that would allow for such a thing but that does not mean it cannot exist. Please note that this counter argument does not apply to the active helper where the action ought to be required even when arbitrary query input is used. > 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. The comment does not explain _why_ we should do nothing so it is 50% useless because it just retells what the code is already saying. The 50% usefulness comes from relaying the fact that the developer actually thought about the non-obvious "else" case (but then decided to hide his reasons for doing nothing in that case). Cheers, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev