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

On 07/18/2013 07:22 AM, Amos Jeffries wrote:
> 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.

I am reposting the fixed patch, sorry :-)

This patch implements all changes requested by Amos. But also add a
simple mechanism to print the list of open configuration files and the
related configuration lines on parse errors. So I am reposting in the
case someone want to comment on it.
I hope this time I did not let debug messages here and there.

Please see my comments bellow.

>
> 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.

ok.

> + 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).

ok

> + 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.

For strtokFile I left the old behaviour. It will print the error, the
line number and the line of the main configuration file.

But I add support for printing all the stack of included files. Example
output:

FATAL: Bungled /home/FS/local/squid3-cvs/etc/squid-newcfg-empty.conf
line 4: Test!
 included from /home/FS/local/squid3-cvs/etc/squid-newcfg-port_8082.conf
line 5: parameters("/home/FS/local/squid3-cvs/etc/squid-newcfg-empty.conf")
 included from /usr/local/squid3-cvs/etc/squid-newcfg.conf line 4:
http_port
parameters("/home/FS/local/squid3-cvs/etc/squid-newcfg-port_8082.conf")
disable-pmtu-discovery=always

I modified for this purpose the ConfigParser::destruct() method.
I hope it is OK.

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

done

>
> * 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.

done

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

ok

>
> * s/funtion/function/
ok

>
> * s/untill/until/
ok

>
> * s/undoed/undone/ or perhapse to be clearer s/ undoed / TokenUndo()
> or TokenPutBack() queued /

ok

> + there are several places using the non-word "undoed" in the patch,
> each need the same.

ok.

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

done

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

done

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

done

>
> * 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/

done

> + please document what happens if UndoToken() is called repeatedly to
> undo multiple NextToken() calls.

Can not be used repeatedly.

>
> * 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 ?

It is a FIFO.

>
> * 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.
> "

ok.

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

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

ok

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

ok

>
> 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") "

ok

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

ok

>
> * please replace" Someone can use single-quoted strings to prevent macro
> substitution." with "Use single-quoted strings to prevent macro
> substitution."

I remove this line. This is not true in the current implementation.
What really this parser does is that if double quotes used, allow
%macros only if the macros supported for this token (eg for logformat).
If single quotes used does not do such check.

> + if possible also example which macros are prevented. (the
> ${process_name} macros? or the %FOO format macros? both?)

>
>
> Amos
>