Message ID | 1330336911-98103-1-git-send-email-martin@martin.st |
---|---|
State | New |
Headers | show |
On 2/27/12 11:01 AM, Martin Storsjö wrote: > This fixes building libavformat with OpenSSL on slightly older > distributions (e.g. CentOS 5.5). > --- > libavformat/tls.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/libavformat/tls.c b/libavformat/tls.c > index fb84fa8..a9e3b27 100644 > --- a/libavformat/tls.c > +++ b/libavformat/tls.c > @@ -177,8 +177,10 @@ static int tls_open(URLContext *h, const char *uri, int flags) > goto fail; > } > SSL_set_fd(c->ssl, c->fd); > +#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME > if (!numerichost) > SSL_set_tlsext_host_name(c->ssl, host); > +#endif > while (1) { > ret = SSL_connect(c->ssl); > if (ret> 0) Ok.
On Mon, 27 Feb 2012, Diego Biurrun wrote: > On Mon, Feb 27, 2012 at 12:01:51PM +0200, Martin Storsjö wrote: >> This fixes building libavformat with OpenSSL on slightly older >> distributions (e.g. CentOS 5.5). >> --- >> libavformat/tls.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) > > CentOS 6 is out. I've been through the pains of supporting the old > 5.5 version with nightmarishly outdated versions of everything at work, > I really don't want to go through the pain again. Do you think this is > worth the trouble? By the time this change has trickled down from HEAD > to the distros, everybody will have updated and those that did not, > should. I occasionally build and test a larger setup including libavformat on such a CentOS machine, and that setup enables the openssl code - this patch fixes building that setup. So for my case, I wouldn't need to wait for this fix to be included in the distros themselves. If you're opposed, I can live without this, but this particular patch is kinda nonintrusive, and I'd probably have written it in this way anyway if I had ran into the issue while making the tls stuff last year. // Martin
On Mon, Feb 27, 2012 at 12:26:45PM +0200, Martin Storsjö wrote: > On Mon, 27 Feb 2012, Diego Biurrun wrote: > > >On Mon, Feb 27, 2012 at 12:01:51PM +0200, Martin Storsjö wrote: > >>This fixes building libavformat with OpenSSL on slightly older > >>distributions (e.g. CentOS 5.5). > >>--- > >> libavformat/tls.c | 2 ++ > >> 1 files changed, 2 insertions(+), 0 deletions(-) > > > >CentOS 6 is out. I've been through the pains of supporting the old > >5.5 version with nightmarishly outdated versions of everything at work, > >I really don't want to go through the pain again. Do you think this is > >worth the trouble? By the time this change has trickled down from HEAD > >to the distros, everybody will have updated and those that did not, > >should. > > I occasionally build and test a larger setup including libavformat > on such a CentOS machine, and that setup enables the openssl code - > this patch fixes building that setup. So for my case, I wouldn't > need to wait for this fix to be included in the distros themselves. So why not change the OpenSSL check to test for the availability of that particular function? > If you're opposed, I can live without this, but this particular > patch is kinda nonintrusive, and I'd probably have written it in > this way anyway if I had ran into the issue while making the tls > stuff last year. To be honest, I disagree slightly about the non-intrusiveness. ifdefs are ifdefs and we should fight them tooth and nail. Diego
On Mon, 27 Feb 2012, Diego Biurrun wrote: > On Mon, Feb 27, 2012 at 12:26:45PM +0200, Martin Storsjö wrote: >> On Mon, 27 Feb 2012, Diego Biurrun wrote: >> >>> On Mon, Feb 27, 2012 at 12:01:51PM +0200, Martin Storsjö wrote: >>>> This fixes building libavformat with OpenSSL on slightly older >>>> distributions (e.g. CentOS 5.5). >>>> --- >>>> libavformat/tls.c | 2 ++ >>>> 1 files changed, 2 insertions(+), 0 deletions(-) >>> >>> CentOS 6 is out. I've been through the pains of supporting the old >>> 5.5 version with nightmarishly outdated versions of everything at work, >>> I really don't want to go through the pain again. Do you think this is >>> worth the trouble? By the time this change has trickled down from HEAD >>> to the distros, everybody will have updated and those that did not, >>> should. >> >> I occasionally build and test a larger setup including libavformat >> on such a CentOS machine, and that setup enables the openssl code - >> this patch fixes building that setup. So for my case, I wouldn't >> need to wait for this fix to be included in the distros themselves. > > So why not change the OpenSSL check to test for the availability of > that particular function? That's of course also an option. (In this case, iirc this isn't a function but a macro, but we can check for the same define as the suggested ifdef does.) >> If you're opposed, I can live without this, but this particular >> patch is kinda nonintrusive, and I'd probably have written it in >> this way anyway if I had ran into the issue while making the tls >> stuff last year. > > To be honest, I disagree slightly about the non-intrusiveness. > ifdefs are ifdefs and we should fight them tooth and nail. Perhaps... But blocking using openssl at all just because we might want to use this optional feature if it is present feels like overkill to me, even if I know it is the norm of how we usually do things. If it is less evil to you, I can move the ifdef to the header of the file to avoid having the ifdef inline. // Martin
diff --git a/libavformat/tls.c b/libavformat/tls.c index fb84fa8..a9e3b27 100644 --- a/libavformat/tls.c +++ b/libavformat/tls.c @@ -177,8 +177,10 @@ static int tls_open(URLContext *h, const char *uri, int flags) goto fail; } SSL_set_fd(c->ssl, c->fd); +#ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME if (!numerichost) SSL_set_tlsext_host_name(c->ssl, host); +#endif while (1) { ret = SSL_connect(c->ssl); if (ret > 0)