Re: [bug-inetutils] [patches 1,2,3] Making tftp/tftpd IPv6-capable.

[Guillem Jover]

Hi!

On Thu, 2010-08-19 at 10:39:04 +0200, Mats Erik Andersson wrote:
> diff --git a/libinetutils/sockaddr_aux.c b/libinetutils/sockaddr_aux.c
> new file mode 100644
> index 0000000..f882f6e
> --- /dev/null
> +++ b/libinetutils/sockaddr_aux.c
> @@ -0,0 +1,98 @@
[...]
> +/* A collection of helpers intended to handle IPv6 and IPv4 simultaneously
> +   in a transparent manner. An underlying use of 'struct sockaddr_storage'
> +   is conceiled as 'struct sockaddr' in the call from an application.
> +
> +   The helper function resolves and manipulates this as 'struct sockaddr_in'
> +   or as 'struct sockaddr_in6', depending on contents.
> +
> +   Mats Erik Andersson, August 2010
> +*/

These helper functions are not needed, and will hinder usage of other
protocols transparently, one of the points of the new inet API is that
it's transparent and does not need to care for the specific protocols
explicitly. See below for detailed comments.

> +in_port_t
> +get_port (struct sockaddr *sa)
> +{
> +  switch (sa->sa_family)
> +    {
> +      case AF_INET:
> +     return ntohs (((struct sockaddr_in *) sa)->sin_port);
> +      case AF_INET6:
> +     return ntohs (((struct sockaddr_in6 *) sa)->sin6_port);
> +      default:
> +     return 0;
> +    }
> +}

> +void
> +set_port (struct sockaddr *sa, in_port_t port)
> +{
> +  switch (sa->sa_family)
> +    {
> +      case AF_INET:
> +     (((struct sockaddr_in *) sa)->sin_port) = htons (port);
> +     break;
> +      case AF_INET6:
> +     (((struct sockaddr_in6 *) sa)->sin6_port) = htons (port);
> +      default:
> +     break;
> +    }
> +}

These two functions can be completely avoided if the port is passed as
the service name in the getaddrinfo calls in the code, it will also
substantially reduce the current code.

> +socklen_t
> +get_socklen (struct sockaddr *sa)
> +{
> +  switch (sa->sa_family)
> +    {
> +      case AF_INET:
> +     return sizeof (struct sockaddr_in);
> +     break;
> +      case AF_INET6:
> +     return sizeof (struct sockaddr_in6);
> +     break;
> +      default:
> +     return sizeof (*sa);
> +     break;
> +    }
> +}

This one is also unneeded, you should just use sockaddr_storage for
storage as it's guaranteed to be able to store any socket address,
then sizeof of the sockaddr_storage type or variable should just
work fine.

> diff --git a/libinetutils/tftpsubs.c b/libinetutils/tftpsubs.c
> index 6eb0a09..371764b 100644
> --- a/libinetutils/tftpsubs.c
> +++ b/libinetutils/tftpsubs.c
> @@ -287,7 +287,7 @@ synchnet (int f)
>  {
>    int i, j = 0;
>    char rbuf[PKTSIZE];
> -  struct sockaddr_in from;
> +  struct sockaddr_storage from;
>    socklen_t fromlen;
>  
>    while (1)

Shouldn't this hunk be in the next patch instead of this one anyway?



> diff --git a/src/tftpd.c b/src/tftpd.c
> index f343f8a..892c709 100644
> --- a/src/tftpd.c
> +++ b/src/tftpd.c
> @@ -103,8 +104,9 @@ static int maxtimeout = 5 * TIMEOUT;
>  #define PKTSIZE      SEGSIZE+4
>  static char buf[PKTSIZE];
>  static char ackbuf[PKTSIZE];
> -static struct sockaddr_in from;
> +static struct sockaddr_storage from;
>  static socklen_t fromlen;
> +static char host[INET6_ADDRSTRLEN];

Probably better to reduce the scope of host and place it inside
verifyhost which is the only user (although keeping it static), which
implies it will be non-reentrant, but that's currently the case anyway.
Also use NI_MAXHOST instead of INET6_ADDRSTRLEN from <netdb.h>.

> @@ -270,24 +272,28 @@ main (int argc, char *argv[])
>       exit (0);
>        }
>    }
> -  from.sin_family = AF_INET;
> +  /* The peer's address 'from' is valid at this point,
> +   * and 'from.ss_family' contains the correct address
> +   * family for any callback connection. */

Maybe place this just before the socket call?

>    alarm (0);
>    close (0);
>    close (1);
> -  peer = socket (AF_INET, SOCK_DGRAM, 0);
> +  peer = socket (from.ss_family, SOCK_DGRAM, 0);
>    if (peer < 0)
>      {
>        syslog (LOG_ERR, "socket: %m\n");
>        exit (1);
>      }




> diff --git a/src/tftp.c b/src/tftp.c
> index f3b3760..e2cfe4a 100644
> --- a/src/tftp.c
> +++ b/src/tftp.c
> @@ -277,26 +266,62 @@ char *hostname;
>  static int
>  resolve_name (char *name, int allow_null)
>  {
> -  struct hostent *hp = gethostbyname (name);
> -  if (hp == NULL)
> +  int rc;
> +  struct sockaddr_storage ss;
> +  struct addrinfo hints, *ai, *aiptr;
> +
> +  memset (&hints, '\0', sizeof (hints));

The structure is made of bytes, not chars, so probably better to use 0
instead of '\0'. Same for the other ones.

> +  hints.ai_family = AF_UNSPEC;
> +  hints.ai_socktype = SOCK_DGRAM;
> +  hints.ai_flags = AI_CANONNAME;
> +#ifdef AI_ADDRCONFIG
> +  hints.ai_flags += AI_ADDRCONFIG;
> +#endif

While this should work just fine, I think it'd be better to use a |=,
as they are bit flags, and thus guarantees that even if AI_ADDRCONFIG
would get added again by accident for example, no wrong results would
happen.

> @@ -338,6 +363,7 @@ setpeer (int argc, char *argv[])
>      case RESOLVE_FAIL:
>        return;
>  
> +#if 0

??

>      case RESOLVE_NOT_RESOLVED:
>        peeraddr.sin_family = AF_INET;
>        peeraddr.sin_addr.s_addr = inet_addr (argv[1]);
> @@ -348,9 +374,10 @@ setpeer (int argc, char *argv[])
>         return;
>       }
>        hostname = xstrdup (argv[1]);
> +#endif

regards,
guillem