Le septidi 7 thermidor, an CCXXIII, Stephan Holljes a écrit : > Attached patch fixes a bug where a oneshot server would not finish a > handshake, because s->handshake_finish was not set.
Thanks for the patch series. I have no remarks about 1-5 and 7, except for
one point of API that I will discuss with my comments on patch 8.
> From fb3cc42c64cc4f9e26dc305e2a3f6aacd6f7a001 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: Implement server side network code.
>
> add http_accept,
> add http_handshake and move handshake logic there,
> handle connection closing.
>
> Signed-off-by: Stephan Holljes <klaxa1...@googlemail.com>
> ---
> libavformat/http.c | 185
> ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 162 insertions(+), 23 deletions(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 676bfd5..0a2662a 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,11 @@
> * path names). */
> #define BUFFER_SIZE MAX_URL_SIZE
> #define MAX_REDIRECTS 8
> +#define HTTP_SINGLE 1
> +#define HTTP_MUTLI 2
> +#define LOWER_PROTO 0
> +#define READ_HEADERS 1
> +#define FINISH 2
At this point, you should probably use enums.
Also, I find the names a bit misleading:
- LOWER_PROTO -> we are doing the lower protocol handshake;
- READ_HEADERS -> we are reading the (request line and) headers;
-> FINISH -> we are about to write the reply status line and headers.
I suggest naming the last one something like "WRITE_REPLY_HEADERS", for
consistency. And also adding a fourth value, maybe "FINISHED" or "DONE", see
below and my next mail.
>
> typedef struct HTTPContext {
> const AVClass *class;
> @@ -97,6 +103,11 @@ typedef struct HTTPContext {
> char *method;
> int reconnect;
> int listen;
> + char *resource;
> + int reply_code;
> + int is_multi_client;
> + int handshake_step;
> + int handshake_finish;
> } HTTPContext;
>
> #define OFFSET(x) offsetof(HTTPContext, x)
> @@ -128,7 +139,10 @@ 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, 2, D | E },
> + { "resource", "The resource requested by a client", OFFSET(resource),
> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
> + { "reply_code", "The http status code to return to a client",
> OFFSET(reply_code), AV_OPT_TYPE_INT, { .i64 = 200}, INT_MIN, 599, E},
> + { "handshake_finish", "Indicate that the protocol handshake is to be
> finished", OFFSET(handshake_finish), AV_OPT_TYPE_INT, { .i64 = 0}, 0, 1, E},
Can you explain why this option is needed? I can see a few corner cases
where it would be useful, but nothing important that warrants worrying about
it now.
Also, I suspect 1 would be a better default value.
> { NULL }
> };
>
> @@ -299,51 +313,152 @@ int ff_http_averror(int status_code, int
> default_averror)
> return default_averror;
> }
>
> +static int http_write_reply(URLContext* h, int status_code)
> +{
> + int ret, body = 0, reply_code;
> + const char *reply_text, *content_type;
> + HTTPContext *s = h->priv_data;
> + char message[BUFFER_SIZE];
> + static const char bad_request[] = "Bad Request";
> + static const char forbidden[] = "Forbidden";
> + static const char not_found[] = "Not Found";
> + static const char internal_server_error[] = "Internal server error";
> + static const char ok[] = "OK";
> + static const char oct[] = "application/octet-stream";
> + static const char text[] = "text/plain";
All these constants are used only once: you could write the strings directly
in place, it produces the same result.
> + content_type = text;
> +
> + if (status_code < 0)
> + body = 1;
> + switch (status_code) {
> + case AVERROR_HTTP_BAD_REQUEST:
> + case 400:
> + reply_code = 400;
> + reply_text = bad_request;
> + break;
> + case AVERROR_HTTP_FORBIDDEN:
> + case 403:
> + reply_code = 403;
> + reply_text = forbidden;
> + break;
> + case AVERROR_HTTP_NOT_FOUND:
> + case 404:
> + reply_code = 404;
> + reply_text = not_found;
> + break;
> + case 200:
> + reply_code = 200;
> + reply_text = ok;
> + content_type = oct;
> + break;
> + case AVERROR_HTTP_SERVER_ERROR:
> + case 500:
> + reply_code = 500;
> + reply_text = internal_server_error;
> + break;
> + default:
> + return AVERROR(EINVAL);
> + }
> + if (body) {
> + s->chunked_post = 0;
> + snprintf(message, sizeof(message),
> + "HTTP/1.1 %03d %s\r\n"
> + "Content-Type: %s\r\n"
> + "Content-Length: %lu\r\n"
%ld is not the correct specifier for size_t, it should be %zu. Well,
%theoretically, because microsoft is always there to mess things up, see
%commit ced0d6c.
(Rule of thumb that probably some people will disagree with: With modern C,
except for interacting with old libraries, if you are using long or short
for something, you are doing it wrong. Use specialized types (size_t, off_t,
ptrdiff_t) or sized types (intXX_t).
> + "\r\n"
> + "%03d %s\r\n",
> + reply_code,
> + reply_text,
> + content_type,
> + strlen(reply_text) + 6, // 3 digit status code + space +
> \r\n
> + reply_code,
> + reply_text);
> + } else {
> + s->chunked_post = 1;
> + snprintf(message, sizeof(message),
> + "HTTP/1.1 %03d %s\r\n"
> + "Content-Type: %s\r\n"
> + "Transfer-Encoding: chunked\r\n"
> + "\r\n",
> + reply_code,
> + reply_text,
> + content_type);
> + }
> + av_log(h, AV_LOG_TRACE, "%s\n", message);
Nit: some introduction and delimiters maybe?
"HTTP reply header: \n%s----\n");
> + if ((ret = ffurl_write(s->hd, message, strlen(message))) < 0)
Nit: you can obtain the message length as the return value of snprintf()
(but only if you are really sure it will fit in the buffer).
> + return ret;
> + if (body)
> + return status_code;
Can you explain?
> + return 0;
> +}
> +
> 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_reply(c, error);
> +}
> +
> +static int http_handshake(URLContext *c)
> +{
> + int ret, err, new_location;
> + HTTPContext *ch = c->priv_data;
> + URLContext *cl = ch->hd;
> + switch (ch->handshake_step) {
> + case LOWER_PROTO:
> + av_log(c, AV_LOG_TRACE, "Lower protocol\n");
> + if ((ret = ffurl_handshake(cl)))
> + return ret;
> + ch->handshake_step = READ_HEADERS;
> + break;
> + case READ_HEADERS:
> + av_log(c, AV_LOG_TRACE, "Read headers\n");
> + if (!ch->end_header) {
I do not understand why this is needed.
> + if ((err = http_read_header(c, &new_location)) < 0) {
new_location made me notice just now that the current code path parses the
headers. It should probably be changed to work differently for clients and
servers, but it can come later.
> + handle_http_errors(c, err);
> + return err;
> + }
> + ch->handshake_step = FINISH;
> + }
> + break;
> + case FINISH:
> + if (ch->handshake_finish) {
> + av_log(c, AV_LOG_TRACE, "Reply code: %d\n", ch->reply_code);
> + if ((err = http_write_reply(c, ch->reply_code)) < 0)
> + return err;
> + return 0;
> }
Nit: I suggest to always put the break, even in the last clause: if another
clause is added later, people will forget to add the break.
> }
> + return 1;
If you do that, remove the part about decreasing value in the doxy for
url_accept. I do not know if decreasing values will actually be useful, but
they are very easy to achieve: just reverse the order of the step constants,
make sure FINISHED/DONE is 0, and use that as a return value.
(Well, technically, 1 1 1 0 is decreasing, just not strictly so, but you get
my meaning.)
> }
>
> 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;
> - s->chunked_post = 1;
> + int port;
> av_url_split(proto, sizeof(proto), NULL, 0, hostname, sizeof(hostname),
> &port,
> NULL, 0, uri);
> if (!strcmp(proto, "https"))
> 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;
> -
> + s->handshake_step = LOWER_PROTO;
> + if (s->listen == HTTP_SINGLE) { /* single client */
> + s->reply_code = 200;
> + s->handshake_finish = 1;
> + while ((ret = http_handshake(h)) > 0);
> + }
> fail:
> - handle_http_errors(h, ret);
> av_dict_free(&s->chained_options);
> return ret;
> }
> @@ -382,6 +497,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 = NULL;
> +
> + av_assert0(sc->listen);
> + if ((ret = ffurl_alloc(c, s->filename, s->flags,
> &sl->interrupt_callback)) < 0)
> + goto fail;
> + cc = (*c)->priv_data;
> + if ((ret = ffurl_accept(sl, &cl)) < 0)
> + goto fail;
> + cc->hd = cl;
> + cc->is_multi_client = 1;
> +fail:
> + return ret;
> +}
> +
> static int http_getc(HTTPContext *s)
> {
> int len;
> @@ -576,7 +711,7 @@ static int process_line(URLContext *h, char *line, int
> line_count,
>
> p = line;
> if (line_count == 0) {
> - if (s->listen) {
> + if (s->listen || s->is_multi_client) {
Do you need to distinguish between multi-client and
single-client-after-handshake? If not, I suggest having a single field
"s->is_server" (or maybe "s->is_connected_server") for both.
> // HTTP method
> method = p;
> while (!av_isspace(*p))
> @@ -597,6 +732,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);
Unchecked memory allocation.
> }
>
> // HTTP resource
> @@ -607,6 +743,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);
Ditto.
>
> // HTTP version
> while (av_isspace(*p))
> @@ -1346,6 +1483,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
