Re: [PATCH] external_acl_type format cleanup pt 1

On 05/06/2014 10:59 AM, Amos Jeffries wrote:
> On 7/05/2014 4:01 a.m., Alex Rousskov wrote:
>> On 05/06/2014 08:37 AM, Amos Jeffries wrote:
>>
>>> Several of the external ACL format codes have been added to
>>> Format::ByteCode_t without equivalent logformat TokenTableEntry's
>>
>>> @ Format::Format::assemble(...)
>>> + // XXX: external_acl_type format codes are not yet output by this code
>>> + case LFT_EXT_ACL_USER_CERT_RAW:
>>> + case LFT_EXT_ACL_USER_CERTCHAIN_RAW:
>>> + case LFT_EXT_ACL_USER_CERT:
>>> + case LFT_EXT_ACL_USER_CA_CERT:
>>> + case LFT_EXT_ACL_CLIENT_EUI48:
>>> + case LFT_EXT_ACL_CLIENT_EUI64:
>>> + case LFT_EXT_ACL_NAME:
>>> + case LFT_EXT_ACL_DATA:
>>
>> I am not versed enough in Format terminology to quickly grok the above,
>> so I have to ask: What does the above mean from the admin point of view?
>> Does the proposed change completely removes support for some external
>> ACL %macros? Or do we still support everything but require some %macro
>> renaming for the old squid.confs to continue to work?
>
> It means they do not yet produce any output if used in config values
> other than external_acl_type. There is also no TokenTableEntry using
> them, so the logformat parser/lexer will also not parse any squid.conf
> token into those codes.

Sounds good. In summary, certain %codes remain specific to the
external_acl_type, but the code is massaged to ease their future wider
support.

> - case _external_acl_format::EXT_ACL_UNKNOWN:
>
> - case _external_acl_format::EXT_ACL_END:
> + default:
> + // TODO: replace this function with Format::assemble()
> + // For now die on unsupported logformat codes.
> fatal("unknown external_acl format error");
> break;
> }

If it is easy, please use fatalf() to report the offending %code name.
That would help folks identifying the offending %code among the many
suspects on a logformat line. This specific change does not warrant
another round of reviews IMO.

Thank you,

Alex.
Received on Tue May 06 2014 - 20:00:18 MDT