Re: [PATCH] Unknown cfg function

On 07/30/2013 10:52 PM, Amos Jeffries wrote:
> On 31/07/2013 6:23 a.m., Tsantilas Christos wrote:
>> On 07/30/2013 08:12 PM, Amos Jeffries wrote:
>>> I just ran your patch past my set of test cases built from the user
>>> complaints so far:
>>>
>>>
>>> 2013/07/31 04:56:39| Processing: configuration_includes_quoted_values
>>> off
>>> ...
>>> 2013/07/31 04:56:39| Processing: acl notAfunction url_regex
>>> ^somethingOLD(.*)
>>> 2013/07/31 04:56:39| Processing: acl notAmacro url_regex "somethingOLD$"
>>> 2013/07/31 04:56:39| Unsupported cfg macro: $"
>>> FATAL: Bungled (#2) strings.conf line 23: acl notAmacro url_regex
>>> "somethingOLD$"
>>>
>> Use single quotes. The single quotes does not try to interpret the '$'
>> character
>>
>> The "configuration_includes_quoted_values" does not disables the macros
>> in quotes.
>> It justs control the quotes behavior for some configuration parameters.
>> The quotes for some alcs for example means read from file. It
>> disables/enables this behavior.
>>
>> However we can extend its behavior.
>
> This is with quoting *disabled*.
>
> It should simply be returning the token "somethingOLD$" quotes and all.

Is not so easy to completely disable quotes if
configuration_includes_quoted_values is set to off.

The goal of supporting quoted tokens is to have a common way in
configuration file for tokens and values. Before this, in some cases
quotes supported, in other cases quotes are not needed.

The patch which enabled quote tokens replaces all strtok and other
similar calls (eg ParseQuotedString) with the ConfigParser::NextToken
call. A simple call for all cases.

For regex looks we need to make an exception.
For some other cases, like logformat we have
ConfigParser::NextQuotedOrToEol which just gets the token until the eol.
Looks that we should fix this method too to not strip "" if
configuration_includes_quoted_values is set to off.
Also I believe in the future the NextQuotedOrToEol will be deprecated,
and users will use always quoted strings, because it is easier to
understand them.

>
>>
>>> Of somewhat less importance there is also this weird case showing up
>>> when I create a bungled http_access line using parameters() in the OFF
>>> mode.
>>> This one is not specific to your patch, it happens with mine too, but it
>>> does worry me...
>>>
>>> 2013/07/31 04:56:39| Processing: configuration_includes_quoted_values
>>> off
>>> ...
>>> 2013/07/31 05:00:41| Processing: http_access parameters("foo")
>>> 2013/07/31 05:00:41| aclParseAccessLine: strings.conf line 18:
>>> http_access parameters("foo")
>>> 2013/07/31 05:00:41| aclParseAccessLine: Access line contains no ACL's,
>>> skipping
>> Which is the problem here? Does the foo file has any values?
>
> This test covering the use of parameters() by someone who forgot to
> enable the new parser mode. In general we want it to fail, but on this
> directive the old parser used to strangely just drop the line without
> even an ERROR highlighting the problem.
>
> The issue I wanted you to notice is that something (probably a forgotten
> strtok or Undo) breaks the config parsing for the next config file line.
> (There used to be no side effects of that ACL parsing bug in the old
> parser.)

OK. I see.
This is because of the strtokFile method used to retrieve ACLs which try
to keep compatibility with old parser.
Looks that the safer is to not allow the use of parameters() if
configuration_includes_quoted_values is set to off.

>
>>
>>> 2013/07/31 05:00:41| Processing: acl notAfunction url_regex
>>> ^somethingOLD(.*)
>>> 2013/07/31 05:00:41| FATAL: Invalid ACL type 'notAfunction' for ACL
>>> named all
>>> FATAL: Bungled (#2) strings.conf line 21: acl notAfunction url_regex
>>> ^somethingOLD(.*)
>> If I use the following:
>> configuration_includes_quoted_values off
>> acl notAfunction url_regex ^somethingOLD(.*)
>> configuration_includes_quoted_values on
>>
>> I am not getting any parse error...
>> Are you sure you are not have any
>> "configuration_includes_quoted_values on"
>> line?
>
> I do and it works perfectly when I comment out the http_access line.
>
> Notice the FATAL message is talking about ACL *type* and displaying the
> *name*.
>

OK. The http_access line problem was causing this problems.
Removing support for parameters() if
configuration_includes_quoted_values set to off solve this in my tests....

> Turning on the level9 debug I get this trace:
>
> 2013/07/31 07:30:07.814| Processing: http_access parameters("foo")
> 2013/07/31 07:30:07.816| src/ConfigParser.cc(391) startParse: Parsing
> from foo
> 2013/07/31 07:30:07.816| aclParseAccessLine: strings.conf line 17:
> http_access parameters("foo")
> 2013/07/31 07:30:07.816| aclParseAccessLine: Access line contains no
> ACL's, skipping
> 2013/07/31 07:30:07.817| ../src/acl/Acl.cc(399) ~ACL: ACL::~ACL:
> 'http_access parameters("foo")'
> 2013/07/31 07:30:07.817| Processing: acl notAmacro url_regex "somethingOLD$
> 2013/07/31 07:30:07.818| src/ConfigParser.cc(287) NextToken:
> CfgFiles.pop foo
>
> ... so "foo" is apparently the ACL name token.
>
> I did some digging and the generated parser is still using a strtok() to
> start the line processing on each new line. That seems to be why the
> "acl" directive name is working.

>
> The major *new* bugs here are that:
> a) parameters("foo") is supposed to be a single invalid SimpleToken
> when the parser is *disabled*.

yes.

> b) the Undo stack has been left containing the "foo" token across an
> end-of-line directive change.

No here.
The probelm was the ConfigParser::strtokFile functon which try to use ""
as filenames to import data, and does not understand included files from
"parameters()"....

I will post a new patch with regex solution and fixes for these problems

Regards,
   Christos

>
> 2013/07/31 07:30:07.818| ../src/acl/Acl.cc(417) Registered:
> ACL::Prototype::Registered: invoked for type notAmacro
> 2013/07/31 07:30:07.818| ../src/acl/Acl.cc(425) Registered:
> ACL::Prototype::Registered: no
> 2013/07/31 07:30:07.818| FATAL: Invalid ACL type 'notAmacro'
>
>
> Amos
>
Received on Wed Jul 31 2013 - 09:02:51 MDT