Re: [PATCH] Improve Configuration Checking and Parsing

On 6/01/2013 1:12 p.m., Tianyin Xu wrote:
> Hi, Amos,
>
> I addressed your audit results one by one (see my reply below each
> audit entry). The updated patches is attached.
>
> Two things I want to mention in the very beginning:
>
> 1. I checked the version according to your information, and made sure
> it's the newest one (revision 12566). :-)
>
> 2. For the precedence level, you mentioned "m*d/u == "m*(d/u)". I
> checked both C and C++ reference manual*,**.
> * M. A. Ellis and B. Stroustrup, "The Annotated C++ Reference
> Manual", Addison-Wesley Publishing, Sept. 1999.
> ** S. P. Harbison and G. L. Steele Jr., "C: A Reference Manual (3rd
> Edition)", Prentice Hall, 1991.
>
> Both of them claims that multiplicative operators (include *, /, %)
> have the same precedence level, and group left-to-right. So I think it
> makes sense to rewrite m*d/u*2 to (m*d/u)*2 but not to m*(d/u)*2. Let
> me know if I misunderstood.

Hmm. Okay. That will do.

>> * at line 3475. port is being set by the strtol() call in the if()
>> condition. There is no need to parse the token twice.
>>
> I kept it.
>
>> * the debugs you are adding at line 3705 appears to already be present in my
>> copy of trunk with better text.
>> Hmm. I'm wondering if you did get trunk checkout properly now.
>>
> I checked the newest version and that one is not there.:-)

Ah found it was a patch I was testing at the time. Sorry.

It added "debugs(3, DBG_CRITICAL, "FATAL: Unknown http(s)_port option '"
<< token << "'");"

>
>> in src/wccp2.cc:
>> * the second chunk is parsing a port value. It should probably be using
>> xatos() and p defined as a uint16_t.
>>
> I kept for the better logging.

audit results round 2:

in src/Parsing.cc:
* xatos() debugs messages are missing spaces after the token output:
     "'cannot be less
    "'is larger than

* spelling ... s/ is similar as / is similar to /

Those are all cosmetic and can be changed on commit along with a
re-format to fix the whitespace.

So, unless there are any others with objections I'm happy to commit
this. +1.

Amos
Received on Thu Jan 10 2013 - 09:04:54 MST