[FFmpeg-devel] [PATCH] lavf/http: Add 301 and 503 error codes to http_write_reply()

[Ganesh Ajjanagadde]

On Wed, Aug 19, 2015 at 9:14 PM, Stephan Holljes
<klaxa1337 at googlemail.com> wrote:
> On Thu, Aug 20, 2015 at 2:39 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> On Wed, Aug 19, 2015 at 7:59 PM, Stephan Holljes
>> <klaxa1337 at googlemail.com> wrote:
>>> On Thu, Aug 20, 2015 at 12:11 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>>> On Wed, Aug 19, 2015 at 12:14 PM, Stephan Holljes
>>>> <klaxa1337 at googlemail.com> wrote:
>>>>> ---
>>>>>  libavformat/http.c | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/libavformat/http.c b/libavformat/http.c
>>>>> index a136918..4dbef3f 100644
>>>>> --- a/libavformat/http.c
>>>>> +++ b/libavformat/http.c
>>>>> @@ -348,11 +348,19 @@ static int http_write_reply(URLContext* h, int status_code)
>>>>>          reply_text = "OK";
>>>>>          content_type = "application/octet-stream";
>>>>>          break;
>>>>> +    case 301:
>>>>> +        reply_code = 301;
>>>>> +        reply_text = "Moved Permanently";
>>>>> +        break;
>>>>
>>>> 301 is usually used for URL redirection,
>>>> and you don't seem to do anything beyond setting the message.
>>>> There needs to be additional logic somewhere to handle this.
>>>> Nevertheless, it is still ok as a patch to me,
>>>> since I assume this will be handled later on.
>>>> I strongly suggest adding something to clarify this in the commit message.
>>>
>>> I did not think about that and so far I have only used it in my
>>> modified ffserver code. What makes a 301 reply different from the
>>> other replies? Maybe I didn't understand the RFC correctly.
>>
>> Quoting from the RFC:
>> "The new permanent URI SHOULD be given by the Location field in the response.
>> Unless the request method was HEAD,
>> the entity of the response SHOULD contain a short hypertext note with
>> a hyperlink to the new URI(s). "
>>
>> My point is that somewhere in the response a new URI must be provided,
>> so that the client can go to the redirected location.
>> I don't know where/how such logic should go, but it needs to be done.
>>
>
> Ah yes, thus far the application had to take care of that. Using the
> location field in HTTPContext seems like a good place to store this
> information. I will work on a patch that includes this. Thanks!

BTW, are you sure line 383 is right?
message_len could be set above sizeof(message) (if we hit the buffer
size limit).
I do not think (but can't confirm easily) that ffurl_write requires
size < sizeof(buf).
See e.g line 1097 - use the strlen function to get the correct message length,
or alternatively (to avoid the strlen call) saturate the return value
of snprintf
to the value you need.

In fact, I don't know why strlen is being used in 1097,
it is an additional call that can be avoided by utilizing return value
of the snprintf.
You might want to look into this and similar instances.

>
>>>
>>>>
>>>>>      case AVERROR_HTTP_SERVER_ERROR:
>>>>>      case 500:
>>>>>          reply_code = 500;
>>>>>          reply_text = "Internal server error";
>>>>>          break;
>>>>> +    case 503:
>>>>> +        reply_code = 503;
>>>>> +        reply_text = "Service Unavailable";
>>>>> +        break;
>>>>
>>>> Looks ok to me.
>>>>
>>>>>      default:
>>>>>          return AVERROR(EINVAL);
>>>>>      }
>>>>> --
>>>>> 2.1.0
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel at ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel