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

[Ganesh Ajjanagadde]

On Wed, Aug 19, 2015 at 10:24 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> 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).

sorry, typo: do not think -> think

> 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