Le tridi 3 thermidor, an CCXXIII, Stephan Holljes a écrit : > From 2dc2be7e8576fd064579d37c75c343a6f18c068c Mon Sep 17 00:00:00 2001 > From: Stephan Holljes <klaxa1...@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 <klaxa1...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
