[MPlayer-dev-eng] [PATCHv2 7/8] stream ftp: Allocate command buffer on-heap

[Alexander Strasser]

Reimar Döffinger wrote:
> On 19 Nov 2012, at 22:24, Alexander Strasser <eclipse7 at gmx.net> wrote:
> > Alexander Strasser wrote:
> >> Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> >> ---
> >> stream/stream_ftp.c | 44 +++++++++++++++++++++++++-------------------
> >> 1 file changed, 25 insertions(+), 19 deletions(-)
> >> 
> > [...]
> >> @@ -434,8 +439,9 @@ static int open_f(stream_t *stream,int mode, void* opts, int* file_format) {
> >> 
> >>   // Allocate buffers
> >>   p->buf = malloc(BUFSIZE);
> >> +  p->cmd_buf = malloc(CMD_BUFSIZE);
> > 
> >  It occurred to me today that I was probably too conservative with
> > this patch.
> > 
> >  If we replace above allocation with something along the lines of:
> > 
> >  p->cmd_buf_size = 5 + FFMIN(strlen(p->filename)+1, 1<<15); // "RETR <file>"
> >  p->cmd_buf = malloc(p->cmd_buf_size);
> 
> I find the FFMIN rather questionable, particularly since the 32 kB it limits to is small enough that there's little point in not always just allocating the maximum.
> Due to memory fragmentation placing it directly in some struct instead of a separate malloc might actually be worth more than making the allocation as small as possible.

  OK! I am not entirely convinced but your view is more pragmatic and
you have a point. Additionally the code is simpler and better for most
(all?) of its current users.

> Using a separate size field might have its advantages though even if it would be a constant for now.

  Do you meant because it could be injected from outside (e.g. via options)?
Then it could be useful for the recv buffer size too. But could also be done
at the point we plan to make if configurable. Did you have anything else in
mind?

> >  - Don't allocate 8k every time we are opening a file via ftp
> 
> I don't really think this matters at all.
> 
> >  - If we are opening a filename longer than ~8k it will still work
> 
> Sure, but  most FTP servers will run e.g. Linux with a PATH_MAX of 4kB, so that is truly unlikely to ever happen.

  When looking at your statements above what I wonder if you would oppose to
just make it an array of size 8k inside the private context struct?

  It would not go so well with configurable buffer sizes I talked about above.
But for now we don't know if we will implement that ever.

  Alexander

P.S.
I will start committing patches 1-6 tomorrow. I would have done it today
but somehow my build of current SVN is broken. Need to investigate that
first...