[MPlayer-dev-eng] [PATCHv2 3/8] stream ftp: readline: Always try to read complete lines

[Reimar Döffinger]

On 18 Nov 2012, at 22:38, Alexander Strasser <eclipse7 at gmx.net> wrote:

> Reimar Döffinger wrote:
>> On Sun, Nov 18, 2012 at 02:12:37PM +0100, Reimar Döffinger wrote:
>>> On Sat, Nov 17, 2012 at 01:01:58AM +0100, Alexander Strasser wrote:
>>>> @@ -144,8 +149,20 @@ static int readline(char *buf,int max,struct stream_priv_s *ctl)
>>>>    }
>>>>       }
>>>>       if (max == 1) {
>>>> -    *buf = '\0';
>>>> -    break;
>>>> +        int found_eol = 0;
>>>> +
>>>> +        while (ctl->cavail > 0) { // skip remaining response line
>>>> +          found_eol = *ctl->cget == '\n';
>>>> +          --ctl->cavail; ++ctl->cget;
>>>> +
>>>> +          if (found_eol) {
>>>> +            break;
>>>> +          }
>>>> +        }
>>>> +
>>>> +        if (found_eol) {
>>>> +      break;
>>>> +        }
>>> 
>>> I find this code quite non-obvious.
>>> Maybe instead
>>> char *eol = memchr(ctl->cget, ctl->cavail);
>>> if (eol) {
>>>  eol++;
>>>  ctl->cavail -= eol - ctl->cget;
>>>  ctl->cget = eol;
>>>  break;
>>> }
>>> // read more data to find end of current line
>>> 
>>> I also wonder if we should do something against the
>>> possibility that a hacked server could keep us in the
>>> loop forever...
>> 
>> I forgot to ask: Did you check it is okay to skip
>> the cget == cput check and resetting all fields code
>> when we break the loop here?
> 
>  I did. I think it is not any different to breaking out
> if we found EOL in the range of the provided buffer in the
> if-block above. On the next call we will get into the
> if (cget == cput)-block if we exhausted the recv buffer on
> the call before. We may get into the changed if (max == 1)-block
> too but it will turn out be a NOP.
> 
>  Talking about that should I change the condition of the
> first if (ctl->cavail > 0)-block to if (ctl->cavail > 0 && max > 1) ?
> And that of the 2nd to if (max == 1) to if (ctl->cavail > 0 && max == 1) ?
> IOW do not try to fill the provided line buffer if it is full already
> and do not try to search for EOL if the recv buffer is exhausted.
> AFAICT it works as well without those extra conditions, so I am not
> really sure about adding them.

I think the code should be fine as is, though it might be possible to make it more obvious. But one thing at a time.

>  Also find attached a text file visualizing some cases with
> the changed behavior. Maybe you can see a flaw in my thinking.

I don't think there is one, I just didn't spend enough time to be sure myself, so I asked if you did.