[FFmpeg-devel] [PATCH 6/8] lavf/http: increase range for listen, handle connection closing accordingly, add http_accept, add http_handshake and move handshake logic there
Nicolas George
george at nsup.org
Tue Jul 21 12:14:19 CEST 2015
Le tridi 3 thermidor, an CCXXIII, Stephan Holljes a écrit :
> From 2dc2be7e8576fd064579d37c75c343a6f18c068c Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Fri, 3 Jul 2015 02:28:56 +0200
> Subject: [PATCH 6/8] lavf/http: increase range for listen, handle connection
> closing accordingly, add http_accept, add http_handshake and move handshake
> logic there
Nit: Git practice advise to have a short first line and add details later,
with lines wrapped not too long (git log adds intendation).
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> libavformat/http.c | 127 ++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 676bfd5..b8016a7 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -25,6 +25,7 @@
> #include <zlib.h>
> #endif /* CONFIG_ZLIB */
>
> +#include "libavutil/avassert.h"
> #include "libavutil/avstring.h"
> #include "libavutil/opt.h"
>
> @@ -44,6 +45,9 @@
> * path names). */
> #define BUFFER_SIZE MAX_URL_SIZE
> #define MAX_REDIRECTS 8
> +#define HTTP_ONESHOT 1
> +#define HTTP_MUTLI 2
> +#define HTTP_MULTI_CLIENT 4
Are they supposed to be flags? Otherwise: where did 3 go?
>
> typedef struct HTTPContext {
> const AVClass *class;
> @@ -97,6 +101,7 @@ typedef struct HTTPContext {
> char *method;
> int reconnect;
> int listen;
> + char *resource;
> } HTTPContext;
>
> #define OFFSET(x) offsetof(HTTPContext, x)
> @@ -128,7 +133,9 @@ static const AVOption options[] = {
> { "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 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 },
> + { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 4, D | E },
Does it make sense for the application/user to set 4? If not, then a
separate field may be better.
> + { "resource", "The resource requested by a client", OFFSET(resource), AV_OPT_TYPE_STRING, { 0 }, 0, 0, E },
> + { "http_code", "The http code to send to a client", OFFSET(http_code), AV_OPT_TYPE_INT, { .i64 = 0}, 0, 599, E},
Nit: Since this is HTTP anyway, the name is redundant. Maybe "reply_code"?
> { NULL }
> };
>
> @@ -299,32 +306,87 @@ int ff_http_averror(int status_code, int default_averror)
> return default_averror;
> }
>
> +static int http_write_header(URLContext* h, int status_code)
The name is misleading: it does not only write the header, it also writes a
single-line reply, except in the 200 case. More on that later.
> +{
> + int ret;
> + const char *message;
> + // Maybe this should be done more elegantly?
> + static const char bad_request[] = "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\nContent-Length: 17\r\n400 Bad Request\r\n";
> + static const char forbidden[] = "HTTP/1.1 403 Forbidden\r\nContent-Type: text/plain\r\nContent-Length: 15\r\n\r\n403 Forbidden\r\n";
> + static const char not_found[] = "HTTP/1.1 404 Not Found\r\nContent-Type: text/plain\r\nContent-Length: 15\r\n\r\n404 Not Found\r\n";
> + static const char internal_server_error[] = "HTTP/1.1 500 Internal server error\r\nContent-Type: text/plain\r\nContent-Length: 25\r\n\r\n500 Internal server error\r\n";
Well, all the reply strings have the following format:
"HTTP/1.1 %03d %s\r\n"
"Content-Type: application/octet-stream\r\n"
"Content-Length: %d\r\n"
"\r\n"
"%03d %s\r\n"
So you can probably have just "int reply_code" and "const char *reply_text"
and fill in in a local buffer.
> + static const char ok[] = "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nTransfer-Encoding: chunked\r\n\r\n";
> + av_log(h, AV_LOG_TRACE, "err: %d\n", status_code);
> + if (status_code == 200) {
> + message = ok;
> + goto end;
> + }
It could go back inside the switch. That avoids the goto.
> + switch(status_code) {
> + case AVERROR_HTTP_BAD_REQUEST:
Nit: usual style is "switch<space>{" and case indented at the same level.
> + message = bad_request;
> + break;
> + case AVERROR_HTTP_FORBIDDEN:
> + message = forbidden;
> + break;
> + case AVERROR_HTTP_NOT_FOUND:
> + message = not_found;
> + break;
> + default:
> + message = internal_server_error;
> + }
> +end:
> + if ((ret = ffurl_write(h, message, strlen(message))) < 0)
> + return ret;
> + // Avoid returning a positive value from ffurl_write()
> + ret = ret > 0 ? 0 : ret;
> + return ret;
If I read the logic correctly, ret is always 0 when it reaches return.
> +}
> +
> static void handle_http_errors(URLContext *h, int error)
> {
> - 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:
> - 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));
> + URLContext *c = s->hd;
> + av_assert0(error < 0);
> + http_write_header(c, error);
> +}
> +
> +static int http_handshake(URLContext *c)
> +{
> + int ret, err, new_location;
> + HTTPContext *ch = c->priv_data;
> + URLContext *cl = ch->hd;
By the way: why "cl"?
> + for (;;) {
> + if ((ret = ffurl_handshake(cl)) < 0)
> + return ret;
> + if (ret == 0)
> + break;
> + }
You are killing the handshake split. The application may want to close the
client immediately, before reading the request, based solely on the IP
address (assuming the TCP protocol returns it, not urgent). Even worse: with
TLS and Server Name Indication, the application must chose a certificate
between two steps of the TLS handshake (not yet implemented, of course). You
must not loop here, you must return each time.
The code could look like something like that:
case (handshake_step) {
case SLAVE_PROTOCOL:
ret = ffurl_handshake(cl);
if (!ret)
handshake_step = READ_HEADERS;
return ret;
case READ_HEADERS:
...
}
> + if (!ch->end_header) {
> + if ((err = http_read_header(c, &new_location)) < 0)
> + goto fail;
If reading the headers fails, IMHO no reply should be sent.
> + }
> + if (ch->http_code) {
> + if (ch->http_code == 200) {
> + http_write_header(cl, 200);
> + return 0;
> }
Does that mean the application must set http_code? That would make it
impossible to make an application that works transparently with several
protocols.
> + err = ff_http_averror(ch->http_code, AVERROR(EIO));
> + goto fail;
If the application has requested an error reply sent to the client, then
from the point of view of the application, this is success.
There is also a protocol concern here: the application can not send a "206
Partial Content" reply, because for anything except 200 the code
automatically sends a single-line body reply. For that matter, sending a
fancy 404 page is impossible too.
I think the following convention could work and be convenient:
- 100 <= http_code < 600 -> return just the header, let the application send
the body;
- http_code < 0 -> translate AVERROR to HTTP as closely as possible and send
a built-in reply body;
- anything else: return AVERROR(EINVAL) immediately.
And 200 should just be the default value.
(And later, someone can work with Andrey to make sure all three-digit codes
map to AVERROR codes; maybe something like that
#define AVERROR_THREE_DIGIT_STATUS(n) FFERRTAG(0xF8, \
'0' + (n) / 100,
'0' + (n) / 10 % 10,
'0' + (n) % 10)
)
> }
> + return 1;
> +fail:
> + handle_http_errors(c, err);
> + return err;
> }
>
> static int http_listen(URLContext *h, const char *uri, int flags,
> AVDictionary **options) {
> 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";
> char hostname[1024], proto[10];
> char lower_url[100];
> const char *lower_proto = "tcp";
> - int port, new_location;
> + int port;
> s->chunked_post = 1;
> av_url_split(proto, sizeof(proto), NULL, 0, hostname, sizeof(hostname), &port,
> NULL, 0, uri);
> @@ -332,18 +394,17 @@ static int http_listen(URLContext *h, const char *uri, int flags,
> lower_proto = "tls";
> ff_url_join(lower_url, sizeof(lower_url), lower_proto, NULL, hostname, port,
> NULL);
> - av_dict_set(options, "listen", "1", 0);
> + if ((ret = av_dict_set_int(options, "listen", s->listen, 0)) < 0)
> + goto fail;
> if ((ret = ffurl_open(&s->hd, lower_url, AVIO_FLAG_READ_WRITE,
> &h->interrupt_callback, options)) < 0)
> goto fail;
> - if ((ret = http_read_header(h, &new_location)) < 0)
> - goto fail;
> - if ((ret = ffurl_write(s->hd, header, strlen(header))) < 0)
> - goto fail;
> - return 0;
> -
> + if (s->listen == HTTP_ONESHOT) { /* single client */
> + s->http_code = 200;
> + // Setting the http_code to 200 should ensure that http_handshake() returns 0.
> + ret = http_handshake(h);
> + }
> fail:
> - handle_http_errors(h, ret);
> av_dict_free(&s->chained_options);
> return ret;
> }
> @@ -382,6 +443,26 @@ static int http_open(URLContext *h, const char *uri, int flags,
> return ret;
> }
>
> +static int http_accept(URLContext *s, URLContext **c)
> +{
> + int ret;
> + HTTPContext *sc = s->priv_data;
> + HTTPContext *cc;
> + URLContext *sl = sc->hd;
> + URLContext *cl;
> + av_assert0(sc->listen);
> + if ((ret = ffurl_alloc(c, s->filename, s->flags & (AVIO_FLAG_READ_WRITE | AVIO_FLAG_DIRECT),
> + &sl->interrupt_callback)) < 0)
> + goto fail;
> + cc = (*c)->priv_data;
> + if ((ret = ffurl_accept(sl, &cl)) < 0)
> + goto fail;
> + cc->hd = cl;
> + cc->listen = HTTP_MULTI_CLIENT;
This is used only once: does it make sense?
> +fail:
> + return ret;
> +}
> +
> static int http_getc(HTTPContext *s)
> {
> int len;
> @@ -597,6 +678,7 @@ static int process_line(URLContext *h, char *line, int line_count,
> "(%s autodetected %s received)\n", auto_method, method);
> return ff_http_averror(400, AVERROR(EIO));
> }
> + s->method = av_strdup(method);
> }
>
> // HTTP resource
> @@ -607,6 +689,7 @@ static int process_line(URLContext *h, char *line, int line_count,
> p++;
> *(p++) = '\0';
> av_log(h, AV_LOG_TRACE, "Requested resource: %s\n", resource);
> + s->resource = av_strdup(resource);
>
> // HTTP version
> while (av_isspace(*p))
> @@ -1346,6 +1429,8 @@ HTTP_CLASS(http);
> URLProtocol ff_http_protocol = {
> .name = "http",
> .url_open2 = http_open,
> + .url_accept = http_accept,
> + .url_handshake = http_handshake,
> .url_read = http_read,
> .url_write = http_write,
> .url_seek = http_seek,
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150721/7dc1086e/attachment.sig>
More information about the ffmpeg-devel
mailing list