[FFmpeg-devel] [PATCH] ipv6-compatible resolve_host() replacement

[Ronald S. Bultje]

Hi Michael,

On 9/29/07, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Sat, Sep 29, 2007 at 12:37:18PM -0400, Ronald S. Bultje wrote:
> > +    for (res = resl, addri = addr; res; addri = addri->next, res =
> res->ai_next) {
>
> there are quite a lot of statements on a single line, i think it would
> be more readable if there where fewer


I need the double variable assignment, though, since we're working on two
linked-lists here. I separated over two lines and aligned the text to be
more readable, I hope this is OK.

> +        addri->next = res->ai_next ? &addri[1] : NULL;
> > +        addri->protocol = res->ai_protocol;
> > +        addri->family = res->ai_family;
> > +        addri->socket_type = res->ai_socktype;
> > +        addri->addr_len = res->ai_addrlen;
> > +        addri->addr = (struct sockaddr *) ptr;
>
> these can be vertically aligned


Fixed (same thing in other places within the function, also).

> +void inetaddr_setport(AVInetAddr *addr, int port)
> > +{
> > +#ifdef CONFIG_IPV6
> > +    if (addr->family == AF_INET6)
> > +        ((struct sockaddr_in6 *) addr->addr)->sin6_port = htons(port);
> > +    else /* AF_INET */
> > +#endif
> > +        ((struct sockaddr_in *) addr->addr)->sin_port = htons(port);
> > +
> > +    addr->port = port;
> > +}
> > +
> > +AVInetAddr *inetaddr_alloc(int socket_stream)
> > +{
> > +    AVInetAddr *addr;
> > +    int extra_size;
> > +#ifdef CONFIG_IPV6
> > +    extra_size = sizeof(struct sockaddr_in6);
> > +#else
> > +    extra_size = sizeof(struct sockaddr_in);
> > +#endif
> > +    addr = av_malloc(sizeof(AVInetAddr) + extra_size);
> > +    addr->addr = (void *) &addr[1];
> > +    addr->addr_len = extra_size;
> > +    addr->next = NULL;
> > +    addr->family = addr->port = addr->protocol = 0;
> > +    addr->socket_type = socket_stream ? SOCK_STREAM : SOCK_DGRAM;
> > +
> > +    return addr;
> > +}
> > +
> > +void inetaddr_reparse(AVInetAddr *addr)
> > +{
> > +    addr->family = addr->addr->sa_family;
> > +#ifdef CONFIG_IPV6
> > +    if (addr->family == AF_INET6)
> > +        addr->port = ntohs(((struct sockaddr_in6 *)
> addr->addr)->sin6_port);
> > +    else
> > +#endif
> > +        addr->port = ntohs(((struct sockaddr_in *)
> addr->addr)->sin_port);
> > +}
>
> these functions are unused
> why are they here?


So as explained, _alloc() is needed to be able to accept() connections on it
(ffserver). Since I added macros to access addr and port within the
sockaddr, I didn't need the port anymore, and family can always be accessed
within the sockaddr, so I removed the other two functions and the
family/port parameters in the AVInetAddr struct.

> +char *inetaddr_str(AVInetAddr *addr, char *buf, int len)
> > +{
> > +#ifdef CONFIG_IPV6
> > +    void *src_ptr;
> > +
> > +    if (addr->family == AF_INET) {
> > +        if (((struct sockaddr_in *) addr->addr)->sin_addr.s_addr ==
> INADDR_ANY) {
> > +            av_strlcpy(buf, " 127.0.0.1", len);
> > +            return buf;
> > +        }
> > +        src_ptr = &((struct sockaddr_in *) addr->addr)->sin_addr;
> > +    } else if (addr->family == AF_INET6) {
> > +        static const struct in6_addr any_addr = IN6ADDR_ANY_INIT;
> > +        if (memcmp (&((struct sockaddr_in6 *) addr->addr)->sin6_addr,
> > +                    &any_addr, sizeof(any_addr))) {
> > +            av_strlcpy(buf, "::1", len);
> > +            return buf;
> > +        }
> > +        src_ptr = &((struct sockaddr_in6 *) addr->addr)->sin6_addr;
> > +    } else
> > +        return NULL;
> > +    if (!inet_ntop(addr->family, src_ptr, buf, len))
> > +        return NULL;
> > +#else
>
> > +    const char *res;
> > +
> > +    if (addr->family != AF_INET)
> > +      return NULL;
> > +    else if (((struct sockaddr_in *) addr->addr)->sin_addr.s_addr ==
> INADDR_ANY) {
>
> please declare this with the proper struct, if needed with a union or
> whatever, these casts all over the code are ugly


I added INETADDR_PORT() for the port and INETADDR_ADDR() to access the
address depending on the family (e.g. as argument to inet_ntop()). For
access to the specific types (in6_addr/in_addr), I added INETADDR_ADDR4()
and *6(). This is used a bit in this patch and a lot in ffserver (see other
thread, but that patch is not completely finished / tested).

> +        av_strlcpy(buf, "127.0.0.1", len);
> > +        return buf;
>
> duplicate


I re-arranged the function so it's no longer duplicate.

> +    } else if (!(res = inet_ntoa(((struct sockaddr_in *)
> addr->addr)->sin_addr)))
> > +        return NULL;
>
> docs say:
>      Instead of `inet_ntoa' the newer function `inet_ntop' which is
>      described below should be used since it handles both IPv4 and IPv6
>      addresses.
>
> so i assume inet_ntop is unavailable on some major os, correct?


_ntop() is only available on systems supporting ipv6, just like
getaddrinfo(). So on systems lacking ipv6, this function lacks also.

> Index: libavformat/avformat.h
> > ===================================================================
> > --- libavformat/avformat.h.orig       2007-09-29 11:22:01.000000000-0400
> > +++ libavformat/avformat.h    2007-09-29 11:38:21.000000000 -0400
> > @@ -21,8 +21,8 @@
> >  #ifndef AVFORMAT_H
> >  #define AVFORMAT_H
> >
> > -#define LIBAVFORMAT_VERSION_INT ((51<<16)+(14<<8)+0)
> > -#define LIBAVFORMAT_VERSION     51.14.0
> > +#define LIBAVFORMAT_VERSION_INT ((51<<16)+(15<<8)+0)
> > +#define LIBAVFORMAT_VERSION     51.15.0
> >  #define LIBAVFORMAT_BUILD       LIBAVFORMAT_VERSION_INT
>
> this is not needed as there is no change to the public API/ABI


OK, removed (that's actually better then).

> + * @param port the port number that will be connected to.
> > + * @param passive 1 if the port will be used to bind() to, else 0.
> > + * @param socket_stream 1 if the returned address will be used for a
> > + *                      TCP/IP stream-based connection, 0 for others.
> > + * @param parse_only 1 if we should only parse numeric IPv4/6 host
> > + *                   addresses without resolving it using a name
> server.
>
> i think it would be more readable to use int flags here that is
>
> blah(0,1,1) is harder to read than blah(AV_RESOLVE_FOO|AV_RESOLVE_BAR)
> as the reader has too lookup what the argument 1,2,3 is to make sense of
> the
> first, while the name in the second makes it immedeatly clear


I have changed this to AV_RESOLVE_STREAM/PARSEONLY/PASSIVE. I also
re-arranged argument order to host, port, flags instead of port, flags, host
since that seemed more logical.

> +/**
> > + * Allocate an AVInetAddr for use in accept().
> > + *
> > + * @param socket_stream 1 if the address is used for TCP/IP
> stream-based
> > +                        connections, 0 for others.
> > + * return the newly allocated AVInetAddr, or NULL on ENOMEM.
>
> @return


Fixed (same bug in _resolve() fixed also).

New patch attached.

Ronald
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg-avinetaddr-ipv6.patch
Type: application/octet-stream
Size: 10985 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071002/999d70bf/attachment.obj>