On Tue, Jul 21, 2015 at 12:14 PM, Nicolas George <geo...@nsup.org> wrote:
> 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?
I wasn't sure if in the future something might want to filter the
different server modes so using 3 would mess up bitmasking. By
introducing a new field as you suggested, this would become obsolete
anyway.
>
>>
>> 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.
This would probably make more sense. I am somewhat hesitant to just
add more and more fields to the HTTPContext; am I worrying too much
about memory footprint here?
>
>> + { "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"?
I merely reused the already present field http_code, should this be avoided?
>
>> { 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.
http_connect() uses s->buffer for the same purpose. Should I just reuse that?
>
>> + 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.
Ok. Should this style be introduced when possible? Then I will make
the switch-style in http.c consistent once this patchset is done.
>
>> + 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.
Yes, I see that now too. Redundant code removed.
>
>> +}
>> +
>> 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"?
_C_lient _L_ower protocol. Is there something better that you can suggest?
While we're at it: What does the "hd" in HTTPContext abbreviate? I
can't think of anything but http descriptor?
>
>> + 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:
> ...
> }
Alright, I didn't think about this enough, probably because I am very
unfamiliar with how handshakes in the lower protocols (especially TLS)
work, I will read up on that.
>
>> + 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.
Right, fixed.
>
>> + }
>
>> + 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.
Right now, yes.
>
>> + 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.
This is a great suggestion, I had trouble deciding where to use HTTP
status codes and where to use AVERROR HTTP errors.
>
> (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?
If cc->listen is not set, process_line() will act like a client and
send client responses to a client. That does not make sense.
The usage of HTTP_MULTI_CLIENT can be substituted by anything but 0
(after re-reading the code, I am fairly certain even 1 can be used),
and as it was mentioned before this is better handled by a separate
field and thus also becomes obsolete.
>
>> +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
>
> _______________________________________________
> 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
