On Fri, 22 May 2015 22:25:53 +0300 (EEST)
Martin Storsjö <[email protected]> wrote:
> On Fri, 22 May 2015, wm4 wrote:
>
> > Move the OpenSSL and GnuTLS implementations to their own files. Other
> > than the connection code (including options) and some boilerplate, no
> > code is actually shared.
>
> Well, the read/write/close functions are (still) pretty much identical
> with just one single function call renamed, but yes, that's not enough for
> keeping things more entangled than necessary.
Alternatively, we could do some sort of TLS abstraction layer (a
cleaner variant of what tls.c is doing now). But then, what the TLS
protocol (in the libavformat sense) does is almost the same: a byte
based filter, just with URLProtocols as input/output.
> > ---
> > There is a minor change in behavior: now the equivalent of TLS_shutdown
> > is called both on normal closing, and closing on the error path. I'm
> > not sure if this matters.
>
> It might be worthwhile to test that it doesn't cause any direct issues
> (e.g. testing with valgrind, if an error can be triggered after c->session
> or c->ssl has been set).
>
> > Also, this assumes lu_zeros patch to make GnuTLS and OpenSSL exclusive
> > has been applied.
> > ---
> > libavformat/Makefile | 4 +-
> > libavformat/tls.c | 427
> > ----------------------------------------------
> > libavformat/tls_common.c | 80 +++++++++
> > libavformat/tls_common.h | 51 ++++++
> > libavformat/tls_gnutls.c | 226 ++++++++++++++++++++++++
> > libavformat/tls_openssl.c | 233 +++++++++++++++++++++++++
> > 6 files changed, 593 insertions(+), 428 deletions(-)
> > delete mode 100644 libavformat/tls.c
> > create mode 100644 libavformat/tls_common.c
> > create mode 100644 libavformat/tls_common.h
> > create mode 100644 libavformat/tls_gnutls.c
> > create mode 100644 libavformat/tls_openssl.c
> >
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 73b9f63..f580d75 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -372,6 +372,8 @@ OBJS-$(CONFIG_YUV4MPEGPIPE_DEMUXER) +=
> > yuv4mpegdec.o
> >
> > # external libraries
> > OBJS-$(CONFIG_LIBRTMP) += librtmp.o
> > +OBJS-$(CONFIG_GNUTLS) += tls_gnutls.o
> > +OBJS-$(CONFIG_OPENSSL) += tls_openssl.o
>
> As James commented, these could be e.g. TLS-OBJS-$(CONFIG), and add
> $(TLS-OBJS-YES) to the list of objects for the tls protocol.
>
> > # protocols I/O
> > OBJS-$(CONFIG_APPLEHTTP_PROTOCOL) += hlsproto.o
> > @@ -400,7 +402,7 @@ OBJS-$(CONFIG_RTP_PROTOCOL) += rtpproto.o
> > OBJS-$(CONFIG_SCTP_PROTOCOL) += sctp.o
> > OBJS-$(CONFIG_SRTP_PROTOCOL) += srtpproto.o srtp.o
> > OBJS-$(CONFIG_TCP_PROTOCOL) += tcp.o
> > -OBJS-$(CONFIG_TLS_PROTOCOL) += tls.o
> > +OBJS-$(CONFIG_TLS_PROTOCOL) += tls_common.o
>
> As James commented elsewhere, this could perhaps be kept as tls.c?
>
>
> In general, this looks good to me.
>
> The only thing I'm left wondering about is relating to this:
>
> > Also, this assumes lu_zeros patch to make GnuTLS and OpenSSL exclusive
> > has been applied.
>
> These libraries are used for other things than implementing the TLS
> protocol - the libraries' crypto stuff is ussed for implementing rtmpe://
> as well. In that case only one of them is actually used, and they work as
> replacements for each other, so it doesn't really hurt, but it's the
> general concept I'm a bit weary about.
>
> I'm trying to think how much work it would be to make this split version
> not fail if more than one is enabled, to see if it'd be worth it:
>
> One way, which OTOH brings back a bit more ifdefs, would be something
> like this:
>
> tls.c:
> #ifdef CONFIG_GNUTLS
> URLProtocol ff_tls_protocol = {
> .name = "tls",
> .url_open2 = tls_gnutls_open,
> .url_read = tls_gnutls_read,
> .url_write = tls_gnutls_write,
> .url_close = tls_gnutls_close,
> .priv_data_size = sizeof(TLSGnutlsContext),
> .flags = URL_PROTOCOL_FLAG_NETWORK,
> .priv_data_class = &tls_class,
> };
> #elif CONFIG_OPENSSL
> ...
>
> This would require exposing the context definitions to tls.c though - not
> pretty.
>
> A separate alternative would be to name the protocol structs differently,
> i.e. naming them ff_tls_gnutls_protocol, ff_tls_openssl_protocol, etc.
> Then you'd have separate register-entries for them in allformats.c, and
> you'd handle the mutual exclusion in configure like this:
> tls_openssl_protocol_deps="!tls_gnutls_protocol" (see rtmp_protocol vs
> librtmp_protocol).
I'd prefer this.
Ideally though, there'd be some way to "redirect" a protocol to a
certain implementation. It could even switch the implementations at
runtime, which might be useful for debugging.
> I.e. it'd treat gnutls/openssl as generic external libraries that lavf can
> use for various tasks, that sure can be enabled at the same time - the
> only thing that you need one single copy of, is an actual implementation
> of the tls:// protocol.
>
> This would probably be a not too large change to your code, and would be
> an even better separation of the implementation (where neither of them
> fight over the same ff_tls_protocol symbol name).
>
> What do you think - worthwhile or not?
>
> // Martin
> _______________________________________________
> libav-devel mailing list
> [email protected]
> https://lists.libav.org/mailman/listinfo/libav-devel
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
