Re: [PATCH] Handle long lines properly

On Mon, 29 Oct 2007, Willy Tarreau wrote:

> On Mon, Oct 29, 2007 at 12:17:30AM +0100, Krzysztof Oledzki wrote:
>>
>>
>> On Mon, 29 Oct 2007, Willy Tarreau wrote:
>> <CUT>
>>> Could you please move this just above MAX_LINE_ARGS, a few lines
>>> below,
>>
>> Of course.
>>
>>> and maybe at the same time increase MAX_LINE_ARGS ?
>>
>> 64? 128?
>
> maybe 64 as a starter. I wouldn't be the one who manipulates 64
> args on a single line!

OK ;)

>>>> @@ -2323,6 +2323,15 @@ int readcfgfile(const char *file)
>>>>
>>>> end = line + strlen(line);
>>>>
>>>> + if (*(end-1) != '\n' && strlen(line) + 1 ==
>>>> sizeof(thisline)) {
>>>> + /* Check if the last char is not \n and we reached
>>>> the limit. Watch out
>>>> + * for the last line without the terminating '\n'!
>>>> + */
>>>
>>> I think that the second part of the check is needed, as it seems to
>>> me that the only case where we can encounter something other than
>>> '\n' in *(end-1) is precisely when the line has been truncated.
>>
>> Some editors (like mcedit for example) are able to create a file without a
>> terminating '\n' on the last line. I know that it is wrong but... So, I
>> would like to distinguish such situation from a truncated line as the
>> application should not reject such files, especially with a misleading
>> error message.
>
> OK, but then I'd like to avoid redoing strlen(line) a second time.
> We could then have something like this :
>
> end = line + strlen(line);
>
> + if ((end-line == sizeof(thisline)-1 && *(end-1) != '\n') {

Redoing strlen(line) is only required in two situations:

• when a line was to long
• on the last, incorrectly termitaned, line

Normally, such situations should not happen. I do not like doing to much math on pointers but if you find it more effective - no problem, I can change it. :)

> Also, my initial fgets() in the while() is awful, it hides an assignment.
> Would you please assign 'line' inside the loop ? It seems like we could
> even declare it there, making code auditing easier.

It seems there are more declarations that can be moved: end, args & arg.

Best regards,

                                 Krzysztof Olêdzki Received on 2007/10/29 00:46