Re: [PATCH] Upgrade process for obsolete options

On 09/23/2010 10:19 AM, Amos Jeffries wrote:
> On 24/09/10 03:57, Alex Rousskov wrote:
>> On 09/23/2010 08:30 AM, Amos Jeffries wrote:
>>> One problem we currently have with upgrades is leaving the parser able
>>> to avoid its bungled/unknown option message for directives which have
>>> been fully removed or massively syntax altered.
>>>
>>> We are able to handle this for flags and option syntax easily but the
>>> parser has been particularly dense and strict on the directives (first
>>> word of each line).
>>>
>>> This patch updates the cf_* and cfgman code to allow a special directive
>>> type "obsolete" which pumps old config lines off to a parse_obsolete()
>>> function for handling without causing the directives to remain in the
>>> publicly visible config documentation.
>>>
>>> cf.data.pre has entries added for all the squid-2.6+ directives I could
>>> find that were removed up to 3.HEAD
>>>
>>> parse_obsolete(char*) is added with tests to catch and report the above
>>> entries as obsolete while ignoring. Several of the directives have
>>> additional explanation text matching the release notes to aid manual
>>> upgrades.
>>
>> There is a clash of approaches here. I think it is best for Squid not to
>> start if the wrong option may significantly affect its functionality.
>
> That is a matter for the parse_obsolete handling of it. This is intended
> for obsolete / useless / ineffective directives.
> The current set are fairly trivial, however its possible to write the
> explanation do something complicated and self_destruct from
> parse_obsolete() as easily as elsewhere in the parser.
>
>> This is especially true during upgrades, when no sane admin would try to
>> upgrade without a backup readily available (even if they did not offline
>> checking). However, I know that many developers prefer when Squid
>> bypasses configuration errors and starts if at all possible instead.
>
> I think I understand what you mean. I've been careful to leave squid
> barfing if actual typos or unknown directives are entered into squid.conf.
>
> This only reduces useless obsolete options to warnings for later
> cleaning up.
>
>>
>> I like the absolete feature as such. I would recommend allowing
>> cf.data.pre COMMENTs for obsolete options and recording upgrade
>> instructions there instead of hard-coding them in parse_obsolete(). A
>> generated obsolete option parser would then display those comments. That
>> would be a much cleaner solution, IMO.
>
> Interesting. I'll think about how to do that without clashing with the
> exports. COMMENTS and DOC_ are a nasty case.

I meant DOC_, not COMMENT_, sorry.

>> I cannot object to the proposed changes even though I think there is a
>> better way to support the same good idea.
>
> Which better way would that be?

It is the way we discussed above. In short, have the upgrade
documentation written in English in .pre rather than in C++ in .cc.

Alex.
Received on Thu Sep 23 2010 - 16:45:15 MDT