Re: How to submit the patches?

Dear Amos,

Thanks a lot for the comments and explanations!!
Let me read the wiki pages and try to do it on the trunk version
instead of the stable version.

Best,
Tianyin

On Thu, Jan 3, 2013 at 7:38 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 2013-01-04 14:50, Tianyin Xu wrote:
>>
>> Hi, all,
>>
>> As proposed earlier, I was working on improving the configuration
>> checking and parsing during the holiday.
>>
>> I mainly focused on two aspects including numeric parsing and enum
>> values by using more strict checkings and more verbosity. I hope to
>> make the software more user-friendly and less error-prone :-)
>> Since all the code are used during user configuration translation, it
>> wouldn't introduce too much overhead.
>>
>> Could anyone tell me how to submit my patches?
>>
>
> Our merge procedure and requirements are all documented here:
> http://wiki.squid-cache.org/MergeProcedure
>
>
>
>> I describe what I did as follows, and I hope it makes sense to you guys.
>>
>> ================
>> 1. Numeric Options
>> ================
>> --------------------------------
>> 1.1. Integer Overflow
>> --------------------------------
>>
>> I mainly modified the parsing functions in squid-3.2.5/src/Parsing.cc
>> and squid-3.2.5/compat/xstrto.cc,
>
>
> FYI: changes built on stable branches such as 3.2 are most likely to have
> problems merging. We will be applying your changes to the 3.HEAD / trunk and
> differences between the branches can cause issues both code bugs on broken
> patching and design problems as the feature differences build up (ie your
> early patches will be in trunk but not 3.2, so your later patches against
> 3.2 will clash with your early ones).
>
>
>>
>> There're several places have integer overflow problems, e.g.,
>>
>> 240 int
>> 241 xatoi(const char *token)
>> 242 {
>> 243 - return xatol(token);
>> 244 + long long input;
>> 245 + int ret;
>> 246 +
>> 247 + input = xatoll(token, 10);
>> 248 + ret = (int) input;
>> 249 +
>> 250 + if (input != (long long) ret) {
>> 251 + debugs(0, 0, "ERROR: The value '" << token << "' is
>> larger than the type 'int'.");
>> 252 + self_destruct();
>> 253 + }
>> 254 +
>> 255 + return ret;
>> 256 +}
>>
>> As this patch illustrated, I mainly check the converted values to make
>> sure it's valid in range.
>>
>> ------------------------------------------------
>> 1.2. String to Number conversion
>> ------------------------------------------------
>>
>> Here, I mainly abandon the direct use of "atoi()", "sscanf()", etc..
>> Squid has already noticed these problem (that's why we have our own
>> xatoi, xatos, etc), so what I do is to make them consistent with what
>> we are expecting. Moreover, some configuration parsing functions still
>> use unsafe code, though we've already had the safe conversion
>> functions. So I simply let it use the available conversion. The
>> following patch is one of the typical modifications:
>>
>> 328 GetInteger64(void)
>> ......
>> 338 - i = strtoll(token, NULL, 10);
>> 339 + input = xatoll(token, 10);
>> ......
>>
>
> We have a compatibility library where these xato*() functions should be
> defined. The versions spread out in the parsing code need to be moved to
> compat/xato.h and compat/xato.cc files. Then we need to provide wrapper
> replacements so the main code can use the "normal" ato*() names and the
> compiler replace with our improved xato*() at build time.
>
> When replacing unsafe functions please ensure the bug in the unsafe one is
> documented as the reason in the commit message. And where relevant please
> add definitions in compat/unsafe.h (with documentation why please) to make
> compile impossible when they are used.
>
>
>> -----------------------------------------
>> 1.3. Signedness conversion
>> -----------------------------------------
>>
>> For signedness issues, I mainly check the patterns which directly
>> assigns a signed value to an unsigned variable, as follows:
>>
>> 165 - s->tcp_keepalive.idle = atoi(t);
>> 166 + s->tcp_keepalive.idle = xatoui(t);
>>
>> In this example, "idle" is defined as "unsigned int" and "t" comes
>> from user input. A negative value (i.e., "-1" to disable the feature)
>> will be converted to a super large positive value if we use atoi()
>> directly.
>
>
> In the cases where -1 is used we want to update the documentation and parser
> to accept the string "none". Which maps internally to the disabled value
> (usually still -1 or -2). After which anything else must be >0.
>
>
>>
>> ============================
>> 2. Enum Options (including Boolean)
>> ============================
>> ----------------------
>> 2.1. Strictness
>> ----------------------
>>
>> For enum options, I mainly strict the if-else checking. I don't know
>> whether this's appropriate.
>>
>> 93 if (!strcasecmp(token, "on") || !strcasecmp(token, "enable"))
>> 94 *var = 1;
>> 95 - else
>> 96 + else if(!strcasecmp(token, "off") || !strcasecmp(token,
>> "disable"))
>> 97 *var = 0;
>> 98 + else {
>> 99 + debugs(0, 0, "ERROR: Invalid option -- Boolean options
>> only accept 'on'/'off' or 'enable'/'disable' to turn on/off the
>> feature.");
>> 100 + self_destruct();
>> 101 + }
>>
>
> It is much like what I would have done. Thank you. Just two notes:
>
> * The debug should probably be: debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT),
> ...
>
> * we generally document on/off as the values for boolean, enable/disable are
> just also-supported values. You can drop enable/disable out of the debugs
> help documentation and probably add them as extra cases with debugs
> informing the admin to update their config to use values on/off.
>
>
>> ---------------------
>> 2.2. Verbosity
>> ---------------------
>>
>> I personally like to give more error logs when users configuration
>> input has something wrong. For example, in parse_access_log(), if none
>> of the keyword can match the user's input, the following message will
>> be printed out:
>>
>> debugs(3, 0, "Log format '" << logdef_name << "' is not defined");
>>
>> So, I add several log message like this one:
>>
>> 133 - else
>> 134 + else {
>> 135 + debugs(0, 0, "ERROR: Invalid option " << token << ":
>> 'uri_whitespace' accepts 'strip', 'deny', 'allow', 'encode', and
>> 'chop'.");
>> 136 self_destruct();
>> 137 + }
>> 138 }
>>
>
> These are fine, but please add the extra verbose debug details with the
> level parameter being DBG_PARSE_NOTE(2) [or maybe something deeper than 2],
> so that they show up with squid -k parse and not on regular reconfigure
> operations.
>
>
> Amos

-- 
Tianyin XU,
http://cseweb.ucsd.edu/~tixu/