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

[Nicolas George]

Le sextidi 16 prairial, an CCXXIII, Stephan Holljes a écrit :
> I hope I understood what you meant.

Exactly.

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

I was a bit confused at first then I noticed. It was just advice for
convenience. Consider you have patch 1/2 introducing an if() and patch 2/2
reindenting. If you discover you need to fix something in patch 1/2, then
you will have to use git rebase or equivalent to swap the fixup commit with
patch 2/2 ; if the fix is near the boundary of the if(), it will even cause
merge conflicts.

For that reason, and since reindent commits are trivial and do not require
many rounds of review, I like to leave them out entirely until the patch
series is almost ok. That way, if I am lucky I can use a simple "git commit
--amend": much time gained.

Of course, you should do whatever is more convenient for your work flow.

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

Good argument.

> From 5ec28499e32ede262c690c556aef5e54fd3ef5d7 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Thu, 4 Jun 2015 01:16:59 +0200
> Subject: [PATCH 1/5] lavf/http: Document method option.
> 
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
>  doc/protocols.texi | 10 ++++++++++
>  libavformat/http.c |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)

LGTM

> From 6e3f560af5f8d9c87154f60c5de0e66eada26b63 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Thu, 4 Jun 2015 01:17:51 +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

> From 500462610f1389a4e8dfa76ca4598500691ea6a7 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Thu, 4 Jun 2015 01:20:28 +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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/libavformat/http.c b/libavformat/http.c
> index e51f524..965967d 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -299,11 +299,28 @@ int ff_http_averror(int status_code, int default_averror)
>          return default_averror;
>  }
>  

> +static void handle_http_errors(URLContext *h, int error) {

The usual coding style for functions is to put the brace on a new line.

> +    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";
> +    HTTPContext *s = h->priv_data;
> +    if (h->is_connected) {

> +        switch(error) {
> +            case AVERROR_HTTP_BAD_REQUEST:

The usual coding style is to indent case with switch, but ff_http_averror()
has the same indent as this, better keep consistency in the file.

> +                ffurl_write(s->hd, bad_request, strlen(bad_request));
> +                break;
> +            default:
> +                av_log(h, AV_LOG_ERROR, "Unhandled HTTP error.\n");
> +                ffurl_write(s->hd, internal_server_error, strlen(internal_server_error));
> +        }
> +    }
> +}
> +

>  static int http_listen(URLContext *h, const char *uri, int flags,
>                         AVDictionary **options) {

I had not noticed the brace last round. It does not matter much.

>      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";
> +

Spurious extra line.

>      char hostname[1024], proto[10];
>      char lower_url[100];
>      const char *lower_proto = "tcp";
> @@ -325,6 +342,7 @@ static int http_listen(URLContext *h, const char *uri, int flags,
>      return 0;
>  
>  fail:
> +    handle_http_errors(h, ret);
>      av_dict_free(&s->chained_options);
>      return ret;
>  }

LGTM apart from the cosmetic changes, probably no need to send an updated
patch just for that.

> From 986e5fe885ec267357855702c3075db1fea944e5 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Thu, 4 Jun 2015 01:21:02 +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 | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)

LGTM

> From 960333650d17dbb1646dbbd236ec535717b7a1eb Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Thu, 4 Jun 2015 01:21:26 +0200
> Subject: [PATCH 5/5] lavf/http: Indent else-clause.
> 
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
>  libavformat/http.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

LGTM

Let us give some time for other people to voice concern, but I believe you
can consider this series is stable and build on it.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150604/8084aa8b/attachment.asc>