Re: [PATCH] v2 upgrade process for obsolete options

On 10/16/2010 07:22 AM, Amos Jeffries wrote:
>
> After Alex suggestion/request the DOC_* comments can be used to
> hard-code the upgrade process documentation in cf.data.pre instead of
> cache_cf.cc.
>
> In total:
>
> 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 squid.conf documentation.
>
> cf.data.pre has entries added for all the 2.6-3.1 directives I could
> find that were removed up to 3.HEAD
>
> allows DOC_START / DOC_END comments to be written in cf.data.pre
> describing the upgrade actions that need to be taken. This text is
> dumped to cache.log verbatim when the configuration option is sighted.
> If "-k parse" is used it is displayed at debug level 0 otherwise it's
> displayed at debug level 1. One line indicating a generic "directive X
> is obsolete" is always displayed at level 0 for backwards compatibility
> with admin expectations of a high level "bungled" message.
>
> After all this text display, parse_obsolete(char*) is called with the
> option name. This function exists in cache_cf.cc and can be coded to
> selectivey do more complex handling of the directive. ie for upgrade
> actions deeper than removal.
>
> One useful side-effect was the removal of some LOC: assert(). This
> allows directives to be configured with no LOC: entry being identical to
> a previously explicit "LOC: none".

Hi Amos,

     I like the overall approach and appreciate the time you took to
document obsolete options. I am also happy this feature required few
code changes.

I am not sure we should add smart upgrading code to src/cache_cf.cc
except under very unusual conditions. I would remove both TODO clauses
from the parse_obsolete() body and rely on cf.data.pre text for upgrade
instructions. I would leave parse_obsolete() API itself, in case we need
it in an emergency. If somebody wants to automate upgrades, they can
write a script. This is a weak preference.

> + Remove this line. Since squid-3.2 configure FTP page display details

s/this line Since/this line. Since/ig

s/Since/Starting with/g [optional]

and add a comma after "Starting with squid-x.y" or "Since squid-x.y".

> +
> +// if (strcmp(ptr, "obsolete") == 0) {
> +// /* add to list of entries */
> +// curr->next = entries;
> +// entries = curr;
> +// }
> +

Remove?

> - gen_parse_alias (name, alias,entry, fp);
> + gen_parse_alias(name, alias, entry, fp);

Avoid whitespace changes.

> (!entry->loc || strcmp(entry->loc, "none") == 0)

This is repeated a few times. Make it an Entry::none() method and/or
remove "LOC: none" from .pre?

+1

Thank you,

Alex.
Received on Tue Oct 19 2010 - 18:09:49 MDT