diff mbox

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

Message ID alpine.LSU.2.20.1901280921130.23637@iq
State Accepted
Headers show

Commit Message

Marton Balint Jan. 28, 2019, 8:23 a.m. UTC 
From bc08c60761df77b37c83a4c285f3ca45e5045979 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@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(-)

Comments

Gyan Doshi Jan. 28, 2019, 8:55 a.m. UTC | #1 
On 28-01-2019 01:53 PM, Marton Balint wrote:
> From bc08c60761df77b37c83a4c285f3ca45e5045979 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@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.

Gyan
Marton Balint Jan. 28, 2019, 9:01 p.m. UTC | #2 
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@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
Carl Eugen Hoyos Jan. 29, 2019, 4:24 a.m. UTC | #3 
2019-01-28 9:23 GMT+01:00, Marton Balint <cus@passwd.hu>:
> From bc08c60761df77b37c83a4c285f3ca45e5045979 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@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.

+1

Carl Eugen
Gyan Doshi Jan. 29, 2019, 4:25 a.m. UTC | #4 
On 29-01-2019 02:31 AM, Marton Balint wrote:
>
>
> 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@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.

Ok, will remove and push.

Gyan
diff mbox

Patch

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.

Regards,
Marton