[FFmpeg-devel] [patch] error codes for http/tcp

[Michael Niedermayer]

Hi

On Sat, Jun 23, 2007 at 10:42:52AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On 6/23/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> >On Fri, Jun 22, 2007 at 08:18:46AM -0400, Ronald S. Bultje wrote:
> >> Index: ffmpeg-mpe/libavformat/tcp.c
> >> ===================================================================
> >> --- ffmpeg-mpe.orig/libavformat/tcp.c 2007-03-22 21:28:04.000000000-0400
> >> +++ ffmpeg-mpe/libavformat/tcp.c      2007-03-22 21:29:17.000000000-0400
> >> @@ -65,17 +65,26 @@
> >>          return AVERROR(ENOMEM);
> >>      h->priv_data = s;
> >>
> >> -    if (port <= 0 || port >= 65536)
> >> +    if (port <= 0 || port >= 65536) {
> >> +        ret = AVERROR(EINVAL);
> >>          goto fail;
> >> +    }
> >>
> >>      dest_addr.sin_family = AF_INET;
> >>      dest_addr.sin_port = htons(port);
> >> -    if (resolve_host(&dest_addr.sin_addr, hostname) < 0)
> >> -        goto fail;
> >> -
> >> +    if (resolve_host(&dest_addr.sin_addr, hostname) < 0) {
> >> +        switch (h_errno) {
> >
> >this is not thread safe (resolve_host() is implemented by gethostbyname()
> >which is thread unsafe and the tread safe gethostbyname_r() does not set
> >h_errno)
> 
> 
> According to [1], some OSes use thread-local memory. I honestly have no idea
> what Linux does (OS X has no *_r() GNU extensions as far as I can see).

 -- Function: struct hostent * gethostbyname (const char *NAME)
     The `gethostbyname' function returns information about the host
     named NAME.  If the lookup fails, it returns a null pointer.
[...]
   The lookup functions above all have one in common: they are not
reentrant and therefore unusable in multi-threaded applications.
Therefore provides the GNU C library a new set of functions which can be
used in this context.


> Anyway, patch attached (I think this is the best you can get, so it won't be
> threadsafe on all platforms, but what can you do...).
> 
> I modified resolve_host() to allow for returning h_errno as in
> gethostbyname_r() and changed tcp.c for the change in the function
> declaration (I'll actually use it in my errno-patch which I'll re-send after
> this is applied). Is this a public API change? I can put an #ifdef version<x
> around it as is done in other places.
> 
> Ronald
> 
> [1] http://osdir.com/ml/org.netlabs.libc.user/2005-12/msg00024.html

> Index: ffmpeg/configure.ac
> ===================================================================
> --- ffmpeg.orig/configure.ac	2007-06-23 09:27:27.000000000 -0400
> +++ ffmpeg/configure.ac	2007-06-23 10:40:57.000000000 -0400
> @@ -411,6 +411,11 @@
>    AC_DEFINE(HAVE_LOCALTIME_R, 1, [whether localtime_r() is available])
>  ])
>  
> +dnl gethostbyname_r check
> +AC_CHECK_FUNC(gethostbyname_r,[
> +  AC_DEFINE(HAVE_GETHOSTBYNAME_R, 1, [whether gethostbyname_r() is available])
> +])
> +
>  dnl lrintf()
>  OLD_LIBS="$LIBS"
>  LIBS="$LIBS $M_LIBS"

this isnt against svn

[...]

> +        if (gethostbyname_r(name, &hp_imp, buf, sizeof(buf), &hp, h_errnop);

may i suggest that you try to compile your code before submitting it?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070623/7669e4be/attachment.pgp>