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

[Reimar Döffinger]

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.
Using a separate size field might have its advantages though even if it would be a constant for now.

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