[FFmpeg-devel] [PATCH 6/8] lavf/http: Implement server side network code.
Stephan Holljes
klaxa1337 at googlemail.com
Tue Jul 28 04:49:33 CEST 2015
On Sat, Jul 25, 2015 at 5:04 PM, Nicolas George <george at nsup.org> wrote:
> 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 <klaxa1337 at 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 <klaxa1337 at 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.
>
I thought it was necessary for the application to indicate it wants to
finish the handshake. I think with the FINAL state this is
superfluous.
>> { 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.
>
Fixed.
>> + 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.
>
Fixed.
> (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");
>
Fixed.
>> + 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).
Okay, since http_write_header() uses a buffer with the same size and
its headers are a lot longer, I am pretty sure the headers will fit.
>
>> + return ret;
>
>> + if (body)
>> + return status_code;
>
> Can you explain?
>
I don't know what I was thinking. After long inspection it seemed to
me like exception based programming. Removed.
>> + 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.
Since http_read_header() returns 0 if headers were already read, this
check is redundant. Removed.
>
>> + 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.
Okay.
>
>> + 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.)
>
I think at first I misunderstood what you meant with the decreasing
return values. I hope I understood it correctly this time.
>> }
>>
>> 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.
New field introduced.
>
>> // 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.
Fixed.
>
>> }
>>
>> // 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.
Fixed.
>
>>
>> // 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
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Regards,
Stephan
More information about the ffmpeg-devel
mailing list