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

[Alexander Strasser]

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.

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

  Alexander
-------------- next part --------------
MPlayer: stream/stream_ftp.c

Modify readline (reads the response from the ftp server) to receive until the
end of line is contained in the receive buffer. Before the receive was bounded
by the size of the line buffer provided to readline. Now we just fill the line
buffer and then still go on until we reach the end of the current response
line.

readline()

                                                                    Notes: - == '\n' (for ASCII art convenience)
          0123456789012345678901234567890123456789012345678901234567890     
line buf |200 Lauter Zeug und viel zu viel zu|                            
recv buf |200 Lauter Zeug und viel zu viel zu viel zu viel-............|
                                       cget->                              cavail == 26, cget == 35
                                                q=memchr->                 q == 48
                                                        q->                q == 49 // ++q
                                                                           cavail == 12 // cavail = cavail - (q - cget)
                                                     cget->                cget == 49 // cget = q
                                                           2109876543210

line buf |200 Lauter Zeug und viel zu viel zu|                                
recv buf |200 Lauter Zeug und viel zu viel zu viel zu viel zu viel zu v|iel-
                                       cget->                              cavail == 26, cget == 35
                                                                           q == NULL // memchr
                                                                  cget->   cget = 61
                                                                           cavail == 0 // reset internal state
recv more data
recv buf |iel-200  End of Response-....................................|
    cget->                                                                 cavail == 61
   q=memchr->                                                              q == 3
           q->                                                             q == 4 // ++q
                                                                           cavail == 57 // cavail = cavail - (q - cget)
        cget->                                                             cget == 4 // cget = q
              7654321098765432109876543210987654321098765432109876543210

line buf |200 Lauter Zeug und viel zu viel zu|                                
recv buf |200 Lauter Zeug und viel zu viel zu viel zu viel zu viel zu -|
                                       cget->                              cavail == 26, cget == 35
                                                             q=memchr->    q == 60
                                                                     q->   q == 61 // ++q
                                                                           cavail == 0  // cavail = cavail - (q - cget)
                                                                  cget->   cget == 61 // cget = q
                                                                       0