[FFmpeg-devel] [PATCH][GSoC] lavf/http: Parse and set HTTP method when listening on HTTP(S)

Stephan Holljes klaxa1337 at googlemail.com
Thu Jun 4 01:26:39 CEST 2015


On Wed, Jun 3, 2015 at 10:39 PM, Nicolas George <george at nsup.org> wrote:
> http_listen(), you wrote it yourself. Maybe I was not clear enough, I just
> suggested to move the code in a function for better readability (the name of
> the function is self-documentating), nothing more complicated.

I hope I understood what you meant.

>> From 3af5c5867672624d45de447d11c2107b3c77742c Mon Sep 17 00:00:00 2001
>> From: Stephan Holljes <klaxa1337 at googlemail.com>
>> Date: Wed, 3 Jun 2015 20:57:53 +0200
>> Subject: [PATCH 5/5] lavf/http: Indent else-clause.
>>
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  libavformat/http.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> This looks ok.
>
> Personally, I have the habit of inserting "/* TODO reindent */" in the code
> where it is needed and only make the reindent patch at the last moment, to
> reduce rebase nuisance.

The order of the patches attached was reversed, maybe that's why it
looked like the indentation was done first.

>
>> From 90f17ce9711304228bc3b7bf8cb4cdfac6b14011 Mon Sep 17 00:00:00 2001
>> From: Stephan Holljes <klaxa1337 at googlemail.com>
>> Date: Wed, 3 Jun 2015 20:57:23 +0200
>> Subject: [PATCH 4/5] lavf/http: Properly process HTTP header on listen.
>>
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  libavformat/http.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 6fad584..dfe2616 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -558,7 +558,7 @@ static int process_line(URLContext *h, char *line, int line_count,
>>                          int *new_location)
>>  {
>>      HTTPContext *s = h->priv_data;
>> -    char *tag, *p, *end;
>> +    char *tag, *p, *end, *method, *resource, *version;
>>      int ret;
>>
>>      /* end of header */
>> @@ -569,6 +569,47 @@ static int process_line(URLContext *h, char *line, int line_count,
>>
>>      p = line;
>>      if (line_count == 0) {
>> +        if (s->listen) {
>> +            // HTTP method
>
>> +            while (av_isspace(*p))
>> +                p++;
>
> I believe spaces are not allowed before the request.

Fixed.

>
>> +            method = p;
>> +            while (!av_isspace(*p))
>> +                p++;
>
>> +            *p = '\0';
>
> Plain 0 is acceptable, your choice.

The rest of the function uses '\0' and I wanted to keep it consistent.

>
>> +            av_log(h, AV_LOG_TRACE, "Received method: %s\n", method);
>> +            if (s->method) {
>> +                if (av_strcasecmp(s->method, method)) {
>> +                    av_log(h, AV_LOG_ERROR, "Received and expected HTTP method do not match. (%s expected, %s received)\n",
>> +                           s->method, method);
>> +                    return ff_http_averror(400, AVERROR(EIO));
>> +                }
>> +            }
>
>> +            p++;
>
> Maybe better keep it grouped with "*p = 0", possibly in a single step:
> *(p++) = 0.

Fixed.

>
>> +
>> +            // HTTP resource
>> +            while (av_isspace(*p))
>> +                p++;
>> +            resource = p;
>> +            while (!av_isspace(*p))
>> +                p++;
>> +            *p = '\0';
>> +            p++;
>> +            av_log(h, AV_LOG_TRACE, "Requested resource: %s\n", resource);
>> +
>> +            // HTTP version
>> +            while (av_isspace(*p))
>> +                p++;
>> +            version = p;
>> +            while (!av_isspace(*p))
>> +                p++;
>> +            *p = '\0';
>> +            if (av_strncasecmp(version, "HTTP/", 5)) {
>> +                av_log(h, AV_LOG_ERROR, "Malformed HTTP version string.\n");
>> +                return ff_http_averror(400, AVERROR(EIO));
>> +            }
>> +            av_log(h, AV_LOG_TRACE, "HTTP version string: %s\n", version);
>> +        } else {
>>          while (!av_isspace(*p) && *p != '\0')
>>              p++;
>>          while (av_isspace(*p))
>> @@ -579,6 +620,7 @@ static int process_line(URLContext *h, char *line, int line_count,
>>
>>          if ((ret = check_http_code(h, s->http_code, end)) < 0)
>>              return ret;
>> +        }
>>      } else {
>>          while (*p != '\0' && *p != ':')
>>              p++;
>> --
>> 2.1.0
>>
>
>> From bbe015d20c1852e7c0c8885ca49a8ba2d926911d Mon Sep 17 00:00:00 2001
>> From: Stephan Holljes <klaxa1337 at googlemail.com>
>> Date: Wed, 3 Jun 2015 20:45:08 +0200
>> Subject: [PATCH 3/5] lavf/http: Rudimentary error handling for HTTP requests
>>  received from clients.
>>
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  libavformat/http.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index e51f524..6fad584 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -304,6 +304,9 @@ static int http_listen(URLContext *h, const char *uri, int flags,
>>      HTTPContext *s = h->priv_data;
>>      int ret;
>>      static const char header[] = "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nTransfer-Encoding: chunked\r\n\r\n";
>> +    static const char bad_request[] = "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n\r\n400 Bad Request\r\n";
>> +    static const char internal_server_error[] = "HTTP/1.1 500 Internal server error\r\nContent-Type: text/plain\r\n\r\n500 Internal server error\r\n";
>> +
>>      char hostname[1024], proto[10];
>>      char lower_url[100];
>>      const char *lower_proto = "tcp";
>> @@ -325,6 +328,16 @@ static int http_listen(URLContext *h, const char *uri, int flags,
>>      return 0;
>>
>>  fail:
>> +    if (h->is_connected) {
>> +        switch(ret) {
>> +            case AVERROR_HTTP_BAD_REQUEST:
>> +                ffurl_write(s->hd, bad_request, strlen(bad_request));
>> +                break;
>> +            default:
>
>> +                av_log(h, AV_LOG_ERROR, "Internal server error.\n");
>
> "Unhandled HTTP error"? Your choice.
>
>> +                ffurl_write(s->hd, internal_server_error, strlen(internal_server_error));
>> +        }
>> +    }
>>      av_dict_free(&s->chained_options);
>>      return ret;
>>  }
>> --
>> 2.1.0
>>
>
>> From a1a13c83724f3b2ec0ee46efbf6e84e123e5538d Mon Sep 17 00:00:00 2001
>> From: Stephan Holljes <klaxa1337 at googlemail.com>
>> Date: Wed, 3 Jun 2015 20:41:50 +0200
>> Subject: [PATCH 2/5] lavf/http: Process HTTP header before sending response.
>>
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  libavformat/http.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> LGTM. Will test when time permits.
>
>> From 40cd84a08ab76fcf5d22e6ca47e3aa3cb4bc0bc1 Mon Sep 17 00:00:00 2001
>> From: Stephan Holljes <klaxa1337 at googlemail.com>
>> Date: Wed, 3 Jun 2015 20:26:37 +0200
>> Subject: [PATCH 1/5] lavf/http: Document method option.
>>
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  doc/protocols.texi | 9 +++++++++
>>  libavformat/http.c | 2 +-
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/protocols.texi b/doc/protocols.texi
>> index f822d81..3a4dbb0 100644
>> --- a/doc/protocols.texi
>> +++ b/doc/protocols.texi
>> @@ -278,6 +278,15 @@ Set initial byte offset.
>>  @item end_offset
>>  Try to limit the request to bytes preceding this offset.
>>
>> + at item method
>> +When used as a client option it sets the HTTP method for the request.
>> +
>> +When used as a server option it sets the HTTP method that is going to be
>> +expected from the client(s).
>> +If the expected and the received HTTP method do not match the client will
>> +be given a Bad Request response.
>
>> +When unset the HTTP method is not checked.
>
> Maybe add something like "not checked for now" or "but this may change", so
> that people will not start relying on it. But this is very minor.

Also fixed.

>
>> +
>>  @item listen
>>  If set to 1 enables experimental HTTP server. This can be used to send data when
>>  used as an output option, or read data from a client with HTTP POST when used as
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 4f6716a..546741a 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -126,7 +126,7 @@ static const AVOption options[] = {
>>      { "location", "The actual location of the data received", OFFSET(location), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D | E },
>>      { "offset", "initial byte offset", OFFSET(off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, INT64_MAX, D },
>>      { "end_offset", "try to limit the request to bytes preceding this offset", OFFSET(end_off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, INT64_MAX, D },
>> -    { "method", "Override the HTTP method", OFFSET(method), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
>> +    { "method", "Override the HTTP method or set the expected HTTP method from a client", OFFSET(method), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E },
>>      { "reconnect", "auto reconnect after disconnect before EOF", OFFSET(reconnect), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },
>>      { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D | E },
>>      { NULL }
>
> LGTM apart from that.
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-http-Document-method-option.patch
Type: text/x-patch
Size: 2317 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/bdb5b158/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-lavf-http-Process-HTTP-header-before-sending-respons.patch
Type: text/x-patch
Size: 1034 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/bdb5b158/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavf-http-Rudimentary-error-handling-for-HTTP-reques.patch
Type: text/x-patch
Size: 2034 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/bdb5b158/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-lavf-http-Properly-process-HTTP-header-on-listen.patch
Type: text/x-patch
Size: 2743 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/bdb5b158/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-lavf-http-Indent-else-clause.patch
Type: text/x-patch
Size: 1468 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/bdb5b158/attachment-0004.bin>


More information about the ffmpeg-devel mailing list