Re: [PATCΗ] Quoted values in squid.conf

Hi Christos,
   So sorry for the long delay in reply. This thread seems to be getting
lost in may mailer filters.

+1. I have another long list of tweaks below, BUT these ones are all
very cosmetic polishing that can be done and comitted without another
review.

Thank you very much for getting this done.

On 1/07/2013 5:21 a.m., Tsantilas Christos wrote:
> I am attaching a new version of the patch.
>
> This patch replaces the file:/path/to/file style for including
> parameters from external files with a function like style:
> parameters("/path/to/file")
>
> The MacroUser class removed . Now no check for valid %macros done inside
> configuration parsing code.
> If a developer need to allow %macros inside quotes for a cfg parameter
> he can use the new
> ConfigParser::EnableMacros()/ConfigParser::DisableMacros() method as
> follows:
>
> ConfigParser::EnableMacros();
> String token = ConfigParser::NextToken();
> ConfigParser::DisableMacros();
>
>
> Notes
> ~~~~
>
> From the final user point of view this patch:
> 1) allow the following configuration:
> # Http port configurations
> http_port 8080 parameters("/etc/squid/http-port-opts.conf")
>
> #old style acls
> configuration_includes_quoted_values off
> acl filed src "/usr/local/squid3-cvs/etc/ip-addresses.txt"
> configuration_includes_quoted_values on
>
> 2) Does not allow inside double quoted strings an escaped $ or %
> character if the parameter does not support macros.

What about users who want to place config files in a directory path with
spaces AND use per-worker ${process_number} macro on the filenames ?

Usually only seen on Windows, which we do not support presently, but
other systems do hit it as well sometimes.

> 3) If someone want to use double quotes inside a quoted string can use
> the escape character:
> logformat mine "%>a %ui %http::un [%tl] \"%rm %ru HTTP/%rv\"
> %http::Hs %\"{Referer}http::>ha"
>
> 4) Unquoted values parsed as before this patch.
>
>
> TODO
> ~~~~
>
> The initial specifications included no macro expansion inside single
> quotes (The ${process_name} and ${process_number} and any other new
> macro will be added should not expand inside single quotes). This is
> requires many changes in configuration file pre-processing code, so it
> is not implemented.

And the audit nits list:

in ConfigParser.cc

* strtokFile()
  + You put an XXX note about adding file:name support. That is now
incorrect, please remove.
  + why are "Token scanned for quote " and "quote found" output at
DBG_CRITICAL ? they seem to be regular operations and would display a
huge amount of unnecessary text on startup and reconfigure. Please
consider droping to a low level (ie 5 or 8).
  + please consider displaying the squid.conf current filename and line
number as a reference where to fix, instead of simply "strtokFile: X not
found" at level-0 debug when fopen() fails. It could be a typo needs
fixing in token X config rule - helping the admin find the line to alter
is useful.

* TokenParse() the line "+ sep = w_space"("; " needs whitespace
between the two string constants or strict parsers will produce errors.

* NextQuotedOrToEol()
  + s/untill //
  + is this really checking for EOF or EOL ? please clarify the comment
in respect to what the method name says.
  + please add a debugs() to explain the self_destruct() in this function.

in ConfigParser.h
* please add an empty line before each "protected:" and "private:" to
improve readability.

* s/funtion/function/

* s/untill/until/

* s/undoed/undone/ or perhapse to be clearer s/ undoed / TokenUndo()
or TokenPutBack() queued /
  + there are several places using the non-word "undoed" in the patch,
each need the same.

* s/set to off then understands the /set to 'off' this interprets /

* docs for LastTokenWasQuoted - s/Return true if the last / \return true
if the last /

* docs for NextQuotedOrToEol - s/Returns the next quoted / \return the
next quoted /

* in TokenUndo() docs you say "next call of this function will return"
but _this_ function is TokenUndo() which has void return.
   + s/of this function/to NextToken() method/
  + please document what happens if UndoToken() is called repeatedly to
undo multiple NextToken() calls.

* in TokenPutBack() please document what will happen if this method is
called multiple times in a row.
  + ie what order do the tokens come back in? FIFO / LIFO ?

* in docs for CfgFile::parse() please replace the last two lines with:
"
    * reads the next line from file if required.
    * \return the body of next element or a NULL pointer if there are no
more token elements in the file.
    * \param type will be filled with the ConfigParse::TokenType for any
element found, or left unchanged if NULL is returned.
"

* in UnQuote() docs please say what happens if end is not NULL.

* in TokenParse() docs replace "Returns ..." line with "\return the next
token, or NULL if there are no available tokens in the nextToken string."

* docs for NextElement() - please use /// for one-liner.

in cf.data.pre:
* please replace the http_port parameters(...) example which will
hopefully be exeedingly rare, with one for ACLs which will be more common
   + such as: " acl whitelist dstdomain
parameters("/etc/squid/whitelist.txt") "

  * please remove the "TODO: " line - preferrably by doing it. That
cf.data.pre text gets published widely.

* please replace" Someone can use single-quoted strings to prevent macro
substitution." with "Use single-quoted strings to prevent macro
substitution."
  + if possible also example which macros are prevented. (the
${process_name} macros? or the %FOO format macros? both?)

Amos
Received on Thu Jul 18 2013 - 04:22:08 MDT