On Sat, Jul 25, 2015 at 5:04 PM, Nicolas George <geo...@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 <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. >
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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Regards, Stephan _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel