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

[Reimar Döffinger]

On Tue, Nov 20, 2012 at 12:33:34AM +0100, Alexander Strasser wrote:
> 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.

Well, I only said "might" anyway. But if you are curious you could
run MPlayer playing even some simple audio file through valgrind's
massif tool and even without cache I think you'll come to the
conclusion that 32 kB just don't matter...

> > 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?

Mostly it just means that everything is in place if you want to change
it.

> > >  - 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?

No I wouldn't mind that, that's basically what I meant by "placing it
directly in some 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.

I see little point in having it configurable really.