[FFmpeg-devel] [PATCH] avformat/http: clarify that ffmpeg will attempt to add missing CRLF

[Marton Balint]

On Mon, 28 Jan 2019, Gyan wrote:

>
>
> On 28-01-2019 01:53 PM, Marton Balint wrote:
>> From bc08c60761df77b37c83a4c285f3ca45e5045979 Mon Sep 17 00:00:00 2001
>> From: Gyan Doshi <ffmpeg at gyani.pro>
>> Date: Mon, 28 Jan 2019 12:20:02 +0530
>> Subject: [PATCH] avformat/http: clarify that ffmpeg will attempt to add
>>  missing CRLF
>>
>> ---
>>  libavformat/http.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 240304f6e6..2f2ce856bc 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -541,10 +541,13 @@ static int http_open(URLContext *h, const char 
>> *uri, int flags,
>>          int len = strlen(s->headers);
>>          if (len < 2 || strcmp("\r\n", s->headers + len - 2)) {
>>              av_log(h, AV_LOG_WARNING,
>> -                   "No trailing CRLF found in HTTP header.\n");
>> +                   "No trailing CRLF found in HTTP header. Adding 
>> it.\n");
>>              ret = av_reallocp(&s->headers, len + 3);
>> -            if (ret < 0)
>> +            if (ret < 0) {
>> +            av_log(h, AV_LOG_ERROR,
>> +                   "Failed to add trailing CRLF.\n");
>>
>> In general I am against adding messages for ENOMEM cases.
>
> The codepath above is contingent upon malformed user input, and not a 
> routine alloc.
> Log message is extended to inform the user we're 
> correcting the header.  So I consider it good practice to report its 
> failure, rather than leaving it opaque.

You do report failure with the returned error code. I really don't see a 
case where it actually helps the user in any way to know that memory 
exhausted there and not before. Therefore the "Failed to add trailing 
CRLF" message is useless.

Regards,
Marton